lkml.org 
[lkml]   [2017]   [Jul]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [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

    \
     
     \ /
      Last update: 2017-07-04 17:59    [W:3.396 / U:0.132 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site