From: Bjorn Helgaas <helgaas@kernel.org>
To: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
Cc: Marc Zyngier <marc.zyngier@arm.com>,
Thomas Gleixner <tglx@linutronix.de>,
linux-pci <linux-pci@vger.kernel.org>,
Linux ARM <linux-arm-kernel@lists.infradead.org>,
LKML <linux-kernel@vger.kernel.org>, Mason <slash.tmp@free.fr>,
Thibaud Cornic <thibaud_cornic@sigmadesigns.com>,
Mark Rutland <mark.rutland@arm.com>,
Ard Biesheuvel <ard.biesheuvel@linaro.org>,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Subject: Re: [PATCH v9 2/3] PCI: Add tango PCIe host bridge support
Date: Tue, 4 Jul 2017 10:58:55 -0500 [thread overview]
Message-ID: <20170704155855.GI13824@bhelgaas-glaptop.roam.corp.google.com> (raw)
In-Reply-To: <16622817-8ccd-107b-be08-676b576f6e8a@sigmadesigns.com>
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
next prev parent reply other threads:[~2017-07-04 15:58 UTC|newest]
Thread overview: 41+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-06-20 8:12 [PATCH v9 0/3] Tango PCIe controller support Marc Gonzalez
2017-06-20 8:14 ` [PATCH v9 1/3] PCI: Add DT binding for tango PCIe controller Marc Gonzalez
2017-06-20 8:17 ` [PATCH v9 2/3] PCI: Add tango PCIe host bridge support Marc Gonzalez
2017-07-02 23:18 ` Bjorn Helgaas
2017-07-03 9:35 ` Ard Biesheuvel
2017-07-03 13:27 ` Bjorn Helgaas
2017-07-04 6:58 ` Jisheng Zhang
2017-07-04 7:16 ` Jisheng Zhang
2017-07-04 8:02 ` Ard Biesheuvel
2017-07-04 8:19 ` Jisheng Zhang
2017-07-04 9:38 ` Ard Biesheuvel
2017-07-05 13:53 ` Joao Pinto
2017-07-03 9:54 ` Marc Gonzalez
2017-07-03 13:13 ` Marc Gonzalez
2017-07-03 15:30 ` Marc Gonzalez
2017-07-04 7:09 ` Peter Zijlstra
2017-07-04 13:08 ` Mason
2017-07-04 14:27 ` Peter Zijlstra
2017-07-04 15:18 ` Mason
2017-07-03 13:40 ` Bjorn Helgaas
2017-07-03 14:34 ` Marc Gonzalez
2017-07-04 15:58 ` Bjorn Helgaas [this message]
2017-07-04 23:42 ` Mason
2017-07-03 18:11 ` Russell King - ARM Linux
2017-07-03 18:44 ` Ard Biesheuvel
2017-07-04 15:15 ` Bjorn Helgaas
2017-07-04 18:17 ` Russell King - ARM Linux
2017-07-04 23:59 ` Mason
2017-07-05 5:21 ` Greg Kroah-Hartman
2017-07-05 12:33 ` Mark Brown
2017-06-20 8:18 ` [PATCH v9 3/3] PCI: Add tango MSI controller support Marc Gonzalez
2017-07-04 20:24 ` [PATCH v9 0/3] Tango PCIe " Bjorn Helgaas
2017-07-04 22:55 ` Mason
2017-07-05 18:03 ` Bjorn Helgaas
2017-07-05 20:39 ` Mason
2017-07-05 21:34 ` Bjorn Helgaas
2017-07-05 21:59 ` Mason
2017-07-06 3:39 ` Bjorn Helgaas
2017-07-06 12:26 ` Mason
2017-07-06 12:40 ` Marc Zyngier
2017-07-06 19:46 ` Bjorn Helgaas
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=20170704155855.GI13824@bhelgaas-glaptop.roam.corp.google.com \
--to=helgaas@kernel.org \
--cc=ard.biesheuvel@linaro.org \
--cc=gregkh@linuxfoundation.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pci@vger.kernel.org \
--cc=marc.zyngier@arm.com \
--cc=marc_gonzalez@sigmadesigns.com \
--cc=mark.rutland@arm.com \
--cc=slash.tmp@free.fr \
--cc=tglx@linutronix.de \
--cc=thibaud_cornic@sigmadesigns.com \
/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: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).