linux-kernel - Re: [PATCH v9 2/3] PCI: Add tango PCIe host bridge support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20170704155855.GI13824@bhelgaas-glaptop.roam.corp.google.com>
Date:   Tue, 4 Jul 2017 10:58:55 -0500
From:   Bjorn Helgaas <helgaas@...nel.org>
To:     Marc Gonzalez <marc_gonzalez@...madesigns.com>
Cc:     Marc Zyngier <marc.zyngier@....com>,
        Thomas Gleixner <tglx@...utronix.de>,
        linux-pci <linux-pci@...r.kernel.org>,
        Linux ARM <linux-arm-kernel@...ts.infradead.org>,
        LKML <linux-kernel@...r.kernel.org>, Mason <slash.tmp@...e.fr>,
        Thibaud Cornic <thibaud_cornic@...madesigns.com>,
        Mark Rutland <mark.rutland@....com>,
        Ard Biesheuvel <ard.biesheuvel@...aro.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Subject: Re: [PATCH v9 2/3] PCI: Add tango PCIe host bridge support

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