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

[Bjorn Helgaas]

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