Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752525AbdGDP67 (ORCPT ); Tue, 4 Jul 2017 11:58:59 -0400 Received: from mail.kernel.org ([198.145.29.99]:51118 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752484AbdGDP66 (ORCPT ); Tue, 4 Jul 2017 11:58:58 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 6151422B54 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=helgaas@kernel.org Date: Tue, 4 Jul 2017 10:58:55 -0500 From: Bjorn Helgaas To: Marc Gonzalez Cc: Marc Zyngier , Thomas Gleixner , linux-pci , Linux ARM , LKML , Mason , Thibaud Cornic , Mark Rutland , Ard Biesheuvel , Greg Kroah-Hartman Subject: Re: [PATCH v9 2/3] PCI: Add tango PCIe host bridge support Message-ID: <20170704155855.GI13824@bhelgaas-glaptop.roam.corp.google.com> References: <987fac41-80dc-f1d0-ec0b-91ae57b91bfd@sigmadesigns.com> <20170702231811.GJ18324@bhelgaas-glaptop.roam.corp.google.com> <79382219-c730-da78-3e5f-5abf3173d7ac@sigmadesigns.com> <20170703134031.GG13824@bhelgaas-glaptop.roam.corp.google.com> <16622817-8ccd-107b-be08-676b576f6e8a@sigmadesigns.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <16622817-8ccd-107b-be08-676b576f6e8a@sigmadesigns.com> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3213 Lines: 88 On Mon, Jul 03, 2017 at 04:34:29PM +0200, Marc Gonzalez wrote: > Bjorn Helgaas wrote: > > > Marc Gonzalez wrote: > > > >> On 03/07/2017 01:18, Bjorn Helgaas wrote: > >> > >>> On Tue, Jun 20, 2017 at 10:17:40AM +0200, Marc Gonzalez wrote: > >>> > >>>> +static int tango_check_pcie_link(void __iomem *test_out) > >>> > >>> I think this is checking for link up. Rename to tango_pcie_link_up() > >>> to follow the convention of other drivers. Take a struct tango_pcie * > >>> instead of an address, if possible. > >> > >> Anything's possible. NB: if I pass the struct, then I have to store > >> the address in the struct, which isn't the case now, since I never > >> need the address later. If you don't mind adding an unnecessary > >> field to the struct, I can do it. What do you say? > > > > The benefit of following the same formula as other drivers is pretty > > large. Most drivers save the equivalent of "base" in the struct. If > > you did that, you wouldn't need an extra pointer; you would just use > > "base + SMP8759_MUX" in the config accessors and "base + SMP8759_TEST_OUT" > > in tango_pcie_link_up(). > > The problem is that TEST_OUT is at 0x74 on SMP8759, but at 0x138 > on my other chip. In fact, all registers have been "reshuffled", > and none have the same offsets on the two chips. > > My solution was to define specific registers in the struct. > > In my [RFC PATCH v0.2] posted March 23, I tried illustrating > the issue: > > +static const struct of_device_id tango_pcie_ids[] = { > + { .compatible = "sigma,smp8759-pcie" }, > + { .compatible = "sigma,rev2-pcie" }, > + { /* sentinel */ }, > +}; > + > +static void smp8759_init(struct tango_pcie *pcie, void __iomem *base) > +{ > + pcie->mux = base + 0x48; > + pcie->msi_status = base + 0x80; > + pcie->msi_mask = base + 0xa0; > + pcie->msi_doorbell = 0xa0000000 + 0x2e07c; > +} > + > +static void rev2_init(struct tango_pcie *pcie, void __iomem *base) > +{ > + void __iomem *misc_irq = base + 0x40; > + void __iomem *doorbell = base + 0x8c; > + > + pcie->mux = base + 0x2c; > + pcie->msi_status = base + 0x4c; > + pcie->msi_mask = base + 0x6c; > + pcie->msi_doorbell = 0x80000000; > + > + writel(lower_32_bits(pcie->msi_doorbell), doorbell + 0); > + writel(upper_32_bits(pcie->msi_doorbell), doorbell + 4); > + > + /* Enable legacy PCI interrupts */ > + writel(BIT(15), misc_irq); > + writel(0xf << 4, misc_irq + 4); > +} > > > Do you agree that the 'base + OFFSET' idiom does not work in > this specific situation? Would you handle it differently? It's definitely a hassle to support chips with different register layouts. Your hardware guys are really making your life hard :) drivers/pci/host/pcie-iproc.c is one strategy. It has iproc_pcie_reg_paxb[] and iproc_pcie_reg_paxb_v2[] with register offsets for different chip versions. It saves a table of register offsets per controller, which is similar to saving a set of pointers per controller. Saving the pointers as you suggest above is marginally more storage but probably easier to read. If the chips are fundamentally different, i.e., if they *operate* differently in addition to having a different register layout, you could make two separate drivers. Bjorn