Hi Arnd,
I am sorry for missed reply style and thank you very much for fixing it.
please see my comments inline.
Thanks,
Minghuan
On 2014年09月05日 08:44, Arnd Bergmann wrote:
On Friday 05 September 2014 07:22:08 Minghuan.Lian@xxxxxxxxxxxxx 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.
[Minghuan] I want to use scfg = <&scfg 0> you mentioned below.
+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.
[Minghuan] I discussed with my colleague. They worry about performance
degradation if using regmap API,
because there are some fast device use scfg. We tend to use a simple way
to map andread/write scfg directly.
+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.
[Minghuan] SCFG does not contains power_on power_off or other PCI
control registers.
We only get link up and MSI related status via SCFG. So I think it is
not enough to call scfg
PCIe phy.
+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.
[Minghuan] I see. I will change it.
Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
