From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752584AbdC0WHj (ORCPT ); Mon, 27 Mar 2017 18:07:39 -0400 Received: from smtpfb1-g21.free.fr ([212.27.42.9]:38204 "EHLO smtpfb1-g21.free.fr" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751962AbdC0WHf (ORCPT ); Mon, 27 Mar 2017 18:07:35 -0400 Subject: Re: [RFC PATCH v0.2] PCI: Add support for tango PCIe host bridge To: Marc Zyngier Cc: Bjorn Helgaas , Thomas Gleixner , Robin Murphy , Lorenzo Pieralisi , Liviu Dudau , David Laight , linux-pci , Linux ARM , Thibaud Cornic , Phuong Nguyen , LKML References: <91db1f47-3024-9712-309a-fb4b21e42028@free.fr> <310db9dd-7db6-2106-2e53-f0083b2d3758@free.fr> <012f7fcb-eaeb-70dd-a1a9-06c213789d30@arm.com> <0502e180-5517-12d6-e3a1-bcea0da7e201@free.fr> <4edd799a-650c-0189-cd5c-e9fc18c5f8bc@arm.com> <8637dyo2d9.fsf@arm.com> From: Mason Message-ID: Date: Tue, 28 Mar 2017 00:04:38 +0200 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:52.0) Gecko/20100101 Firefox/52.0 SeaMonkey/2.49 MIME-Version: 1.0 In-Reply-To: <8637dyo2d9.fsf@arm.com> Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 27/03/2017 23:07, Marc Zyngier wrote: > On Mon, Mar 27 2017 at 08:44:08 PM, Mason wrote: > >> On 27/03/2017 19:09, Marc Zyngier wrote: >> >>> On 27/03/17 16:53, Mason wrote: >>> >>>> I have one remaining issue with bitmaps. >>>> >>>> My HW regs are 32b. How do I grab e.g. bits 96-127? >>>> All I can think of is >>>> u32 val = ((u32 *)bitmap)[3]; >>>> >>>> Is this acceptable? >>> >>> No. >>> >>>> mrutland mentioned bitmap_to_u32array() but IIUC it is used to >>>> copy an entire bitmap. >>> >>> The real question is "Why do you need such a thing?". >> >> You told me to use an in-memory version of the "unmasked" >> bitmap, yes? In that case, I need to be able to grab >> a piece of said bitmap, to update the corresponding piece >> of the HW bitmap. >> >> For example, assume the first 3 MSIs are unmasked, >> in other words, unmasked = 0x7 >> >> A new user comes along and wants to assign an MSI. >> Scan "unmasked", found bit at pos 3. >> Update "unmasked" to 0xf. >> At some point, I need to write 0xf to some HW reg. >> So I need to grab a piece of "unmasked" (bits 0-31 in my example) >> >> pos = find_first_zero_bit(unmasked, COUNT); >> __set_bit(pos, unmasked); >> int reg_index = pos / 32; >> u32 val = ((u32 *)unmasked)[reg_index]; >> writel_relaxed(val, pcie->enabled + reg_index * 4); >> >> Or did I miss something in your suggestion? > > I don't know, I'm sightly taken aback by your question. Completely > puzzled, actually. "Read Modify Write" is a fairly obvious construct. > > val = readl_relaxed(pcie->enabled + reg_index); > writel_relaxed(val | BIT(pos % 32), pcie->enabled + reg_index); > > I never realized this could be such a novel concept. Replace pos with > hwirq, add a spinlock, and that's your irq_unmask. I'm not sure why you would think I'm not familiar with RMW. My original code was: mask = readl_relaxed(pcie->msi_mask); pos = find_first_zero_bit(&mask, 32); writel(mask | BIT(pos), pcie->msi_mask); To which you replied: "Do you really need to read this from the HW each time you allocate an interrupt? That feels pretty crazy. You're much better off having an in-memory bitmap that will make things more efficient" This is why I was trying to avoid a MMIO read from the HW each time I allocate an interrupt. > My suggestion was to use a bitmap in order not to perform extra MMIO > accesses on the fast path. It doesn't mean that you cannot read from the > register under any circumstances. It just means that you don't do it > when there are more efficient ways to do it. The fast path is the interrupt handler, right? (I didn't read the interrupt mask in the ISR.) I understand your underlying point. It is fine to read from MMIO in the slow path, but try as hard as possible NOT to do so in the fast path. Regards. From mboxrd@z Thu Jan 1 00:00:00 1970 From: slash.tmp@free.fr (Mason) Date: Tue, 28 Mar 2017 00:04:38 +0200 Subject: [RFC PATCH v0.2] PCI: Add support for tango PCIe host bridge In-Reply-To: <8637dyo2d9.fsf@arm.com> References: <91db1f47-3024-9712-309a-fb4b21e42028@free.fr> <310db9dd-7db6-2106-2e53-f0083b2d3758@free.fr> <012f7fcb-eaeb-70dd-a1a9-06c213789d30@arm.com> <0502e180-5517-12d6-e3a1-bcea0da7e201@free.fr> <4edd799a-650c-0189-cd5c-e9fc18c5f8bc@arm.com> <8637dyo2d9.fsf@arm.com> Message-ID: To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On 27/03/2017 23:07, Marc Zyngier wrote: > On Mon, Mar 27 2017 at 08:44:08 PM, Mason wrote: > >> On 27/03/2017 19:09, Marc Zyngier wrote: >> >>> On 27/03/17 16:53, Mason wrote: >>> >>>> I have one remaining issue with bitmaps. >>>> >>>> My HW regs are 32b. How do I grab e.g. bits 96-127? >>>> All I can think of is >>>> u32 val = ((u32 *)bitmap)[3]; >>>> >>>> Is this acceptable? >>> >>> No. >>> >>>> mrutland mentioned bitmap_to_u32array() but IIUC it is used to >>>> copy an entire bitmap. >>> >>> The real question is "Why do you need such a thing?". >> >> You told me to use an in-memory version of the "unmasked" >> bitmap, yes? In that case, I need to be able to grab >> a piece of said bitmap, to update the corresponding piece >> of the HW bitmap. >> >> For example, assume the first 3 MSIs are unmasked, >> in other words, unmasked = 0x7 >> >> A new user comes along and wants to assign an MSI. >> Scan "unmasked", found bit at pos 3. >> Update "unmasked" to 0xf. >> At some point, I need to write 0xf to some HW reg. >> So I need to grab a piece of "unmasked" (bits 0-31 in my example) >> >> pos = find_first_zero_bit(unmasked, COUNT); >> __set_bit(pos, unmasked); >> int reg_index = pos / 32; >> u32 val = ((u32 *)unmasked)[reg_index]; >> writel_relaxed(val, pcie->enabled + reg_index * 4); >> >> Or did I miss something in your suggestion? > > I don't know, I'm sightly taken aback by your question. Completely > puzzled, actually. "Read Modify Write" is a fairly obvious construct. > > val = readl_relaxed(pcie->enabled + reg_index); > writel_relaxed(val | BIT(pos % 32), pcie->enabled + reg_index); > > I never realized this could be such a novel concept. Replace pos with > hwirq, add a spinlock, and that's your irq_unmask. I'm not sure why you would think I'm not familiar with RMW. My original code was: mask = readl_relaxed(pcie->msi_mask); pos = find_first_zero_bit(&mask, 32); writel(mask | BIT(pos), pcie->msi_mask); To which you replied: "Do you really need to read this from the HW each time you allocate an interrupt? That feels pretty crazy. You're much better off having an in-memory bitmap that will make things more efficient" This is why I was trying to avoid a MMIO read from the HW each time I allocate an interrupt. > My suggestion was to use a bitmap in order not to perform extra MMIO > accesses on the fast path. It doesn't mean that you cannot read from the > register under any circumstances. It just means that you don't do it > when there are more efficient ways to do it. The fast path is the interrupt handler, right? (I didn't read the interrupt mask in the ISR.) I understand your underlying point. It is fine to read from MMIO in the slow path, but try as hard as possible NOT to do so in the fast path. Regards.