On Fri, Sep 04, 2015 at 09:29:14AM +0100, Ley Foon Tan wrote:

On Wed, Sep 2, 2015 at 12:33 AM, Lorenzo Pieralisi
[off-list ref] wrote:

[...]

quoted

quoted

+static bool altera_pcie_valid_config(struct altera_pcie *pcie,
+                                    struct pci_bus *bus, int dev)
+{
+       /* If there is no link, then there is no device */
+       if (bus->number != pcie->root_bus_nr) {
+               if (!altera_pcie_link_is_up(pcie))
+                       return false;
+       }

Can you explain to pls me why you have to check this for every config
transaction ? Isn't it something that can prevent probing the
host controller altogether ?

In our PCIe hardware spec, it stated that software should check the
link status before issuing a configuration request to downstream
ports.
BTW, other pci controllers have similar implementation as well, eg: dw
pci, mvebu pci.

Understood, thanks.

[...]

quoted

quoted

+static int tlp_read_packet(struct altera_pcie *pcie, u32 *value)
+{
+       u8 loop;
+       struct tlp_rp_regpair_t tlp_rp_regdata;
+
+       for (loop = 0; loop < TLP_LOOP; loop++) {
+               tlp_read_rx(pcie, &tlp_rp_regdata);
+               if (tlp_rp_regdata.ctrl & RP_RXCPL_EOP) {
+                       if (value)
+                               *value = tlp_rp_regdata.reg0;
+                       return PCIBIOS_SUCCESSFUL;
+               }
+               udelay(5);

Could you comment please on the chosen udelay/TLP_LOOP values (ie how
did you come up with them) ?

For udelay value, we just want to have small delay between each read.

I would explain how you chose the value, in particular if it can
affect next generation hosts sharing the same driver.

For TLP_LOOP value, minimum 2 loops to read tlp headers and 1 loop to
read data payload. So, we choose to poll 10 loops for maximum.

Add it to a comment.

[...]

quoted

quoted

+static int altera_pcie_probe(struct platform_device *pdev)
+{
+       struct altera_pcie *pcie;
+       struct pci_bus *bus;
+       int ret;
+
+       pcie = devm_kzalloc(&pdev->dev, sizeof(*pcie), GFP_KERNEL);
+       if (!pcie)
+               return -ENOMEM;
+
+       pcie->pdev = pdev;
+
+       ret = altera_pcie_parse_dt(pcie);
+       if (ret) {
+               dev_err(&pdev->dev, "Parsing DT failed\n");
+               return ret;
+       }
+
+       INIT_LIST_HEAD(&pcie->resources);
+
+       ret = altera_pcie_parse_request_of_pci_ranges(pcie);
+       if (ret) {
+               dev_err(&pdev->dev, "Failed add resources\n");
+               return ret;
+       }
+
+       ret = altera_pcie_init_irq_domain(pcie);
+       if (ret) {
+               dev_err(&pdev->dev, "Failed creating IRQ Domain\n");
+               return ret;
+       }
+
+       pcie->root_bus_nr = 0;

Nit: It is already 0.

Okay. Will remove it.

quoted

quoted

+
+       /* clear all interrupts */
+       cra_writel(pcie, P2A_INT_STS_ALL, P2A_INT_STATUS);
+       /* enable all interrupts */
+       cra_writel(pcie, P2A_INT_ENA_ALL, P2A_INT_ENABLE);
+
+       bus = pci_scan_root_bus(&pdev->dev, pcie->root_bus_nr, &altera_pcie_ops,
+                               pcie, &pcie->resources);
+       if (!bus)
+               return -ENOMEM;
+
+       pci_fixup_irqs(pci_common_swizzle, of_irq_parse_and_map_pci);
+       pci_assign_unassigned_bus_resources(bus);

I think you are missing a call to pcie_bus_configure_settings(),
see drivers/pci/host/pci-host-generic.c

Other pci controller drivers like xgene and iproc don't call to this
function, but it call in
arch/arm/kernel/bios32.c:pci_common_init_dev().
Do we really need this?

It is there to provide a way to configure the system MPS, through
command line parameters, it is always a good idea to have it and
it should be part of your driver so that you can tune it in case
the MPS is misconfigured or you want to apply a specific configuration
for a given system.

Have a look at pcie_bus_config and how it is used in
pcie_bus_configure_settings(), that would clarify further.

Thanks,
Lorenzo