答复: [PATCH 2/2] PCI: Layerscape: Add Layerscape PCIe driver

[Arnd Bergmann]

On Friday 05 September 2014 07:22:08 Minghuan.Lian at freescale.com wrote:
> Hi Arnd,
> 
> Please see my comments inline.

Please use the usual reply style in the future, using '>' characters to
properly give attribution. I'm fixing this up manually in my reply
here, so others can follow the discussion.

> > On Thursday 04 September 2014 18:45:38 Minghuan Lian wrote:
> > > +
> > > +#define PCIE_LS1021A_BASE    0x3400000
> > > +#define PCIE_LS1021A_REG_SIZE        0x0100000
> > 
> > This does not belong here. All addresses must come from DT,
> > and you should not do the calculation below based on the address.
> [Minghuan] LS1021A contains two PCI controllers. I use them to 
> calculation the PCI controller index.
> There are several separate registers for PEX1 and PEX2 in SCFG
> register space. It is hard to get them address from DTS. Could
> you tell me a way to get PCI controller index?

The easiest way would be to put the index as a property in the
device tree itself.

> > +struct ls_pcie {
> > +     struct list_head node;
> > +     struct device *dev;
> > +     struct pci_bus *bus;
> > +     void __iomem *dbi;
> > +     void __iomem *scfg;
> > +     struct pcie_port pp;
> > +     int id;
> > +     int index;
> > +     int irq;
> > +     int msi_irq;
> > +     int pme_irq;
> > +};
> 
> irq and pme_irq seem to be completely unused, so better
> not add them until they are actually used.
> [Minghuan] Ok, I will remove them.
> 
> The scfg registers seem to belong to another device that
> is responsible for multiple instances. Unfortunately,
> this "fsl,ls1021a-scfg" device is not documented anywhere
> that I can see.
> 
> Is this some sort of syscon node, or is it specific to the
> pcie controller(s)?
> 
> [Minghaun] SCFG provides SoC specific configuration and status
> registers for the device including PCI USB eTSEC SATA ...
> The platform patches that contains SCFG code are being upstream.

This sounds like it should be a syscon node, and you should use
syscon_regmap_lookup_by_phandle() to find the scfg node from each
of those drivers, rather than scanning the DT yourself in each
of the drivers using it.

This can also be a place to put the index, such as 

	scfg = <&scfg 0>;

for the first PCIe node. The probe function then extracts both
the phandle for the syscon and the index from that property.

> > +static int ls_pcie_link_up(struct pcie_port *pp)
> > +{
> > +     u32 rc, tmp;
> > +     struct ls_pcie *pcie = to_ls_pcie(pp);
> > +
> > +     tmp = ioread32(pcie->scfg + SCFG_SCFGREVCR_OFF);
> > +     iowrite32(SCFG_NO_BIT_REVERSE, pcie->scfg + SCFG_SCFGREVCR_OFF);
> > +
> > +     rc = (ioread32(pcie->scfg + PEXMSCPORTSR(pcie->index)) >>
> > +          LTSSM_STATE_SHIFT) & LTSSM_STATE_MASK;
> > +
> > +     iowrite32(tmp, pcie->scfg + SCFG_SCFGREVCR_OFF);
> > +
> > +     if (rc < LTSSM_PCIE_L0)
> > +             return 0;
> > +
> > +     return 1;
> > +}
> 
> This looks like it is really a phy driver, although a pretty minimal
> one.
> 
> [Minghuan] I read SCFG register. SCFG provides SoC specific configuration
> and status registers for the device including PCI USB eTSEC SATA ...

I'm guessing that a lot of that is phy related information though.
A nicer way to deal with this, compared to using syscon directly is
to have a phy driver for your chip, and have that deal with all
the phy related setup. In this case you would have a reference in
the client driver like

	phys = <&scfgphy LS1021A_PHY_PCIE0>;
	phy-names = "pcie";

And in the pcie driver you just call devm_phy_get(dev, "pcie") to get a reference
to that phy node, and then you can use the phy API to perform actions on
it (init, power_on, power_off, exit).

This keeps all knowledge about the phy registers inside of the scfg area
in one place, and you only have to add a new phy driver for a future SoC
that has the same PCIe core but different scfg.

> > +static u32 ls_pcie_get_msi_addr(struct pcie_port *pp)
> > +{
> > +     return SCFG_SPIMSICR_ADDR;
> > +}
> > +
> > +static u32 ls_pcie_get_msi_data(struct pcie_port *pp, int pos)
> > +{
> > +     struct ls_pcie *pcie = to_ls_pcie(pp);
> > +
> > +     if (pcie->id == PCI_DEVICE_ID_LS1021A)
> > +             return MSI_LS1021A_DATA(pcie->index);
> > +
> > +     return pos;
> > +}
> 
> My impression is that you have two distinct MSI controller
> implementations, one for LS1021A and the other one for everything
> else. How about using separate pcie_host_ops for them, possibly
> also moving them into separate files?
> 
> A good way to deal with this is to put the pointers to the two
> pcie_host_ops into the data fields of the ls_pcie_of_match table.
> 
> [Minghuan] LS1021 contains the same two PCI controllers PEX1 and PEX2.
> both PEX1 and PEX use the same MSI address, but different MSI message data.

I mean LS1021 is different from other chips here, since you compare
the pcie->id value.

	Arnd