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
