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
> <lorenzo.pieralisi@xxxxxxx> wrote:
[...]
> > > +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.
[...]
> > > +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.
[...]
> > > +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.
> >
> > > +
> > > + /* 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
--
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
