From: "TK, Pratheesh Gangadhar" <pratheesh@ti.com> To: Thomas Gleixner <tglx@linutronix.de> Cc: "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, "hjk@hansjkoch.de" <hjk@hansjkoch.de>, "gregkh@suse.de" <gregkh@suse.de>, "sshtylyov@mvista.com" <sshtylyov@mvista.com>, "arnd@arndb.de" <arnd@arndb.de>, "Chatterjee, Amit" <amit.chatterjee@ti.com>, "davinci-linux-open-source@linux.davincidsp.com" <davinci-linux-open-source@linux.davincidsp.com>, "linux-arm-kernel@lists.infradead.org" <linux-arm-kernel@lists.infradead.org> Subject: RE: [PATCH v7 1/1] PRUSS UIO driver support Date: Wed, 2 Mar 2011 14:17:42 +0530 [thread overview] Message-ID: <B85A65D85D7EB246BE421B3FB0FBB593024BF36776@dbde02.ent.ti.com> (raw) In-Reply-To: <alpine.LFD.2.00.1103012238360.2701@localhost6.localdomain6> Hi, > From: Thomas Gleixner [mailto:tglx@linutronix.de] > Sent: Wednesday, March 02, 2011 3:15 AM > > + > > +static DEFINE_SPINLOCK(lock); > > +static struct clk *pruss_clk; > > +static struct uio_info *info; > > +static dma_addr_t sram_paddr, ddr_paddr; > > +static void *prussio_vaddr, *sram_vaddr, *ddr_vaddr; > > + > > +static irqreturn_t pruss_handler(int irq, struct uio_info *info) > > +{ > > + int intr_bit = (irq - IRQ_DA8XX_EVTOUT0 + 2); > > + int val, intr_mask = (1 << intr_bit); > > + void __iomem *base = info->mem[0].internal_addr; > > + void __iomem *intren_reg = base + PINTC_HIER; > > + void __iomem *intrstat_reg = base + PINTC_HIPIR + (intr_bit << 2); > > + > > + spin_lock_irq(&lock); > > No, I said: spin_lock() is sufficient. Ok. > > + val = ioread32(intren_reg); > > + /* Is interrupt enabled and active ? */ > > + if (!(val & intr_mask) && (ioread32(intrstat_reg) & HIPIR_NOPEND)) { > > + spin_unlock_irq(&lock); > > You unconditinally enable interrupts here where you are not supposed > to do so. > Ok. > > + return IRQ_NONE; > > + } > > + > > + /* Disable interrupt */ > > + iowrite32((val & ~intr_mask), intren_reg); I checked more on this and actually INTC h/w has Host Interrupt Enable Indexed Set Register (HIEISR) and Host Interrupt Enable Indexed Clear Register(HIEICR) which I can use to enable/disable interrupts without doing RMW. I will use these registers and then we don't need all the spinlock and irqcontrol stuff. So I need to do iowrite32((intr_bit, HIEICR);// This disable the interrupt bit in intern_reg. Userspace can use HIEISR to re-enable the interrupt. > > + spin_unlock_irq(&lock); > > + return IRQ_HANDLED; > > +} > > + > > +static int pruss_irqcontrol(struct uio_info *info, s32 irq_on) > > +{ > > + int intr_bit = info->irq - IRQ_DA8XX_EVTOUT0 + 2; > > + int val, intr_mask = (1 << intr_bit); > > + void __iomem *base = info->mem[0].internal_addr; > > + void __iomem *intren_reg = base + PINTC_HIER; > > + > > + spin_lock_irq(&lock); > > This one is correct, as this is always called from non interrupt > disabled context. > > > + val = ioread32(intren_reg); > > + if (irq_on) > > + iowrite32((val | intr_mask), intren_reg); > > + else > > + iowrite32((val & ~intr_mask), intren_reg); > > + spin_unlock_irq(&lock); > > + > > + return 0; > > +} > > > > + > > + spin_lock_init(&lock); > > Sigh. DEFINE_SPINLOCK(lock); already initializes the lock. > > It's not the purpose of a review to tell you what you need to change > mechanically. Reviewers hint to a correct solution and you are > supposed to lookup what that solution means and act accordingly. If > you do not understand the hint or its implications please ask _before_ > sending a new patch set. Seriously, I went to "fix the comments" mode. Sorry about that. Anyway I learnt more about things by making mistakes i.e. the positive side. Thanks a lot for helping us improve on this. Pratheesh
WARNING: multiple messages have this Message-ID (diff)
From: pratheesh@ti.com (TK, Pratheesh Gangadhar) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH v7 1/1] PRUSS UIO driver support Date: Wed, 2 Mar 2011 14:17:42 +0530 [thread overview] Message-ID: <B85A65D85D7EB246BE421B3FB0FBB593024BF36776@dbde02.ent.ti.com> (raw) In-Reply-To: <alpine.LFD.2.00.1103012238360.2701@localhost6.localdomain6> Hi, > From: Thomas Gleixner [mailto:tglx at linutronix.de] > Sent: Wednesday, March 02, 2011 3:15 AM > > + > > +static DEFINE_SPINLOCK(lock); > > +static struct clk *pruss_clk; > > +static struct uio_info *info; > > +static dma_addr_t sram_paddr, ddr_paddr; > > +static void *prussio_vaddr, *sram_vaddr, *ddr_vaddr; > > + > > +static irqreturn_t pruss_handler(int irq, struct uio_info *info) > > +{ > > + int intr_bit = (irq - IRQ_DA8XX_EVTOUT0 + 2); > > + int val, intr_mask = (1 << intr_bit); > > + void __iomem *base = info->mem[0].internal_addr; > > + void __iomem *intren_reg = base + PINTC_HIER; > > + void __iomem *intrstat_reg = base + PINTC_HIPIR + (intr_bit << 2); > > + > > + spin_lock_irq(&lock); > > No, I said: spin_lock() is sufficient. Ok. > > + val = ioread32(intren_reg); > > + /* Is interrupt enabled and active ? */ > > + if (!(val & intr_mask) && (ioread32(intrstat_reg) & HIPIR_NOPEND)) { > > + spin_unlock_irq(&lock); > > You unconditinally enable interrupts here where you are not supposed > to do so. > Ok. > > + return IRQ_NONE; > > + } > > + > > + /* Disable interrupt */ > > + iowrite32((val & ~intr_mask), intren_reg); I checked more on this and actually INTC h/w has Host Interrupt Enable Indexed Set Register (HIEISR) and Host Interrupt Enable Indexed Clear Register(HIEICR) which I can use to enable/disable interrupts without doing RMW. I will use these registers and then we don't need all the spinlock and irqcontrol stuff. So I need to do iowrite32((intr_bit, HIEICR);// This disable the interrupt bit in intern_reg. Userspace can use HIEISR to re-enable the interrupt. > > + spin_unlock_irq(&lock); > > + return IRQ_HANDLED; > > +} > > + > > +static int pruss_irqcontrol(struct uio_info *info, s32 irq_on) > > +{ > > + int intr_bit = info->irq - IRQ_DA8XX_EVTOUT0 + 2; > > + int val, intr_mask = (1 << intr_bit); > > + void __iomem *base = info->mem[0].internal_addr; > > + void __iomem *intren_reg = base + PINTC_HIER; > > + > > + spin_lock_irq(&lock); > > This one is correct, as this is always called from non interrupt > disabled context. > > > + val = ioread32(intren_reg); > > + if (irq_on) > > + iowrite32((val | intr_mask), intren_reg); > > + else > > + iowrite32((val & ~intr_mask), intren_reg); > > + spin_unlock_irq(&lock); > > + > > + return 0; > > +} > > > > + > > + spin_lock_init(&lock); > > Sigh. DEFINE_SPINLOCK(lock); already initializes the lock. > > It's not the purpose of a review to tell you what you need to change > mechanically. Reviewers hint to a correct solution and you are > supposed to lookup what that solution means and act accordingly. If > you do not understand the hint or its implications please ask _before_ > sending a new patch set. Seriously, I went to "fix the comments" mode. Sorry about that. Anyway I learnt more about things by making mistakes i.e. the positive side. Thanks a lot for helping us improve on this. Pratheesh
next prev parent reply other threads:[~2011-03-02 8:48 UTC|newest] Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top 2011-03-01 21:28 [PATCH v7 0/1] Add PRUSS UIO driver support Pratheesh Gangadhar 2011-03-01 21:28 ` Pratheesh Gangadhar 2011-03-01 21:28 ` [PATCH v7 1/1] " Pratheesh Gangadhar 2011-03-01 21:28 ` Pratheesh Gangadhar 2011-03-01 21:45 ` Thomas Gleixner 2011-03-01 21:45 ` Thomas Gleixner 2011-03-01 22:22 ` Hans J. Koch 2011-03-01 22:22 ` Hans J. Koch 2011-03-02 8:47 ` TK, Pratheesh Gangadhar [this message] 2011-03-02 8:47 ` TK, Pratheesh Gangadhar 2011-03-02 9:27 ` Thomas Gleixner 2011-03-02 9:27 ` Thomas Gleixner
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=B85A65D85D7EB246BE421B3FB0FBB593024BF36776@dbde02.ent.ti.com \ --to=pratheesh@ti.com \ --cc=amit.chatterjee@ti.com \ --cc=arnd@arndb.de \ --cc=davinci-linux-open-source@linux.davincidsp.com \ --cc=gregkh@suse.de \ --cc=hjk@hansjkoch.de \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=sshtylyov@mvista.com \ --cc=tglx@linutronix.de \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.