Re: [PATCH v6 3/6] pci:host: Add Altera PCIe host controller driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Sep 2, 2015 at 12:33 AM, Lorenzo Pieralisi
<lorenzo.pieralisi@xxxxxxx> wrote:
>
> On Tue, Sep 01, 2015 at 11:30:05AM +0100, Ley Foon Tan wrote:
>
> [...]
>
> > +static void altera_pcie_retrain(struct pci_dev *dev)
> > +{
> > +       u16 linkcap, linkstat;
> > +
> > +       /*
> > +        * Set the retrain bit if the PCIe rootport support > 2.5GB/s, but
> > +        * current speed is 2.5 GB/s.
> > +        */
> > +       pcie_capability_read_word(dev, PCI_EXP_LNKCAP, &linkcap);
> > +
> > +       if ((linkcap & PCI_EXP_LNKCAP_SLS) <= PCI_EXP_LNKCAP_SLS_2_5GB)
> > +               return;
> > +
> > +       pcie_capability_read_word(dev, PCI_EXP_LNKSTA, &linkstat);
> > +       if ((linkstat & PCI_EXP_LNKSTA_CLS) == PCI_EXP_LNKSTA_CLS_2_5GB)
> > +               pcie_capability_set_word(dev, PCI_EXP_LNKCTL,
> > +                                        PCI_EXP_LNKCTL_RL);
> > +}
> > +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_ALTERA, PCI_ANY_ID, altera_pcie_retrain);
>
> This filtering is still arguable, since it unconditionally applies to
> all Altera PCI devices (I guess there are not any apart from this
> host controller).
This fixup is required for all Altera PCIe controller in all device families.
>
> > +
> > +static void altera_pcie_fixup_res(struct pci_dev *dev)
> > +{
> > +       /*
> > +        * Prevent enumeration of root port.
> > +        */
> > +       if (!dev->bus->parent && dev->devfn == 0) {
> > +               int i;
> > +
> > +               for (i = 0; i < PCI_NUM_RESOURCES; i++) {
> > +                       dev->resource[i].start = 0;
> > +                       dev->resource[i].end   = 0;
> > +                       dev->resource[i].flags   = 0;
> > +               }
> > +       }
> > +}
> > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_ALTERA, PCI_ANY_ID,
> > +                        altera_pcie_fixup_res);
>
> Ditto, it is a quirk of your host controller, not a quirk for
> all Altera PCI devices.
Same here.

>
> > +static inline void cra_writel(struct altera_pcie *pcie, u32 value, u32 reg)
> > +{
> > +       writel_relaxed(value, pcie->cra_base + reg);
> > +}
> > +
> > +static inline u32 cra_readl(struct altera_pcie *pcie, u32 reg)
> > +{
> > +       return readl_relaxed(pcie->cra_base + reg);
> > +}
> > +
> > +static void tlp_read_rx(struct altera_pcie *pcie,
> > +                       struct tlp_rp_regpair_t *tlp_rp_regdata)
> > +{
> > +       tlp_rp_regdata->ctrl = cra_readl(pcie, RP_RXCPL_STATUS);
> > +       tlp_rp_regdata->reg0 = cra_readl(pcie, RP_RXCPL_REG0);
> > +       tlp_rp_regdata->reg1 = cra_readl(pcie, RP_RXCPL_REG1);
> > +}
> > +
> > +static void tlp_write_tx(struct altera_pcie *pcie,
> > +                        struct tlp_rp_regpair_t *tlp_rp_regdata)
> > +{
> > +       cra_writel(pcie, tlp_rp_regdata->reg0, RP_TX_REG0);
> > +       cra_writel(pcie, tlp_rp_regdata->reg1, RP_TX_REG1);
> > +       cra_writel(pcie, tlp_rp_regdata->ctrl, RP_TX_CNTRL);
> > +}
> > +
> > +static bool altera_pcie_link_is_up(struct altera_pcie *pcie)
> > +{
> > +       return !!(cra_readl(pcie, RP_LTSSM) & LTSSM_L0);
> > +}
> > +
> > +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.
>
> > +
> > +       /* access only one slot on each root port */
> > +       if (bus->number == pcie->root_bus_nr && dev > 0)
> > +               return false;
> > +
> > +       /*
> > +        * Do not read more than one device on the bus directly attached
> > +        * to RC.
>
> You should also explain why.
Okay.

>
> > +        */
> > +       if (bus->primary == pcie->root_bus_nr && dev > 0)
> > +               return false;
> > +
> > +        return true;
> > +}
> > +
> > +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.
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.

>
> > +       }
> > +
> > +       return -ENOENT;
> > +}
> > +
> > +static void tlp_write_packet_unaligned(struct altera_pcie *pcie, u32 *headers,
> > +                                      u32 data)
> > +{
> > +       struct tlp_rp_regpair_t tlp_rp_regdata;
> > +
> > +       tlp_rp_regdata.reg0 = headers[0];
> > +       tlp_rp_regdata.reg1 = headers[1];
> > +       tlp_rp_regdata.ctrl = RP_TX_SOP;
> > +       tlp_write_tx(pcie, &tlp_rp_regdata);
> > +
> > +       tlp_rp_regdata.reg0 = headers[2];
> > +       tlp_rp_regdata.reg1 = data;
> > +       tlp_rp_regdata.ctrl = RP_TX_EOP;
> > +       tlp_write_tx(pcie, &tlp_rp_regdata);
> > +}
> > +
> > +static void tlp_write_packet_aligned(struct altera_pcie *pcie, u32 *headers,
> > +                                    u32 data)
> > +{
> > +       struct tlp_rp_regpair_t tlp_rp_regdata;
> > +
> > +       tlp_rp_regdata.reg0 = headers[0];
> > +       tlp_rp_regdata.reg1 = headers[1];
> > +       tlp_rp_regdata.ctrl = RP_TX_SOP;
> > +       tlp_write_tx(pcie, &tlp_rp_regdata);
> > +
> > +       tlp_rp_regdata.reg0 = headers[2];
> > +       tlp_rp_regdata.reg1 = 0;
> > +       tlp_rp_regdata.ctrl = 0;
> > +       tlp_write_tx(pcie, &tlp_rp_regdata);
> > +
> > +       tlp_rp_regdata.reg0 = data;
> > +       tlp_rp_regdata.reg1 = 0;
> > +       tlp_rp_regdata.ctrl = RP_TX_EOP;
> > +       tlp_write_tx(pcie, &tlp_rp_regdata);
> > +}
>
> I do not think you need two functions here, a flag can do.
Okay, will merge these 2 functions.
>
> > +static int tlp_cfg_dword_read(struct altera_pcie *pcie, u8 bus, u32 devfn,
> > +                             int where, u32 *value)
> > +{
> > +       int ret;
> > +       u32 headers[TLP_HDR_SIZE];
> > +
> > +       if (bus == pcie->root_bus_nr)
> > +               headers[0] = TLP_CFG_DW0(TLP_FMTTYPE_CFGRD0);
> > +       else
> > +               headers[0] = TLP_CFG_DW0(TLP_FMTTYPE_CFGRD1);
> > +
> > +       headers[1] = TLP_CFG_DW1(TLP_REQ_ID(pcie->root_bus_nr, devfn),
> > +                                       TLP_READ_TAG);
> > +       headers[2] = TLP_CFG_DW2(bus, devfn, where);
> > +
> > +       tlp_write_packet_unaligned(pcie, headers, 0);
> > +
> > +       ret = tlp_read_packet(pcie, value);
> > +       if (ret != PCIBIOS_SUCCESSFUL)
> > +               *value = ~0UL;  /* return 0xFFFFFFFF if error */
> > +
> > +       return ret;
> > +}
> > +
> > +static int tlp_cfg_dword_write(struct altera_pcie *pcie, u8 bus, u32 devfn,
> > +                              int where, u32 value)
> > +{
> > +       u32 headers[TLP_HDR_SIZE];
> > +       int ret;
> > +
> > +       if (bus == pcie->root_bus_nr)
> > +               headers[0] = TLP_CFG_DW0(TLP_FMTTYPE_CFGWR0);
> > +       else
> > +               headers[0] = TLP_CFG_DW0(TLP_FMTTYPE_CFGWR1);
> > +
> > +       headers[1] = TLP_CFG_DW1(TLP_REQ_ID(pcie->root_bus_nr, devfn),
> > +                                       TLP_WRITE_TAG);
> > +       headers[2] = TLP_CFG_DW2(bus, devfn, where);
> > +
> > +       /* check alignment to Qword */
> > +       if ((where & 0x7) == 0)
> > +               tlp_write_packet_aligned(pcie, headers, value);
> > +       else
> > +               tlp_write_packet_unaligned(pcie, headers, value);
> > +
> > +       ret = tlp_read_packet(pcie, NULL);
> > +       if (ret != PCIBIOS_SUCCESSFUL)
> > +               return ret;
> > +
> > +       /* Keep an eye out for changes to the root bus number */
> > +       if ((bus == pcie->root_bus_nr) && (where == PCI_PRIMARY_BUS))
> > +               pcie->root_bus_nr = (u8)(value);
>
> Essentially you are polling the rootport bridge programming here, correct ?
> I think you should describe this a bit better along with the assumptions
> you are making.
Yes, you are right. Will update description here.
>
[...]

> > +
> > +static void altera_pcie_release_of_pci_ranges(struct altera_pcie *pcie)
> > +{
> > +       pci_free_resource_list(&pcie->resources);
> > +}
> > +
> > +static int altera_pcie_parse_request_of_pci_ranges(struct altera_pcie *pcie)
> > +{
> > +       int err, res_valid = 0;
> > +       struct device *dev = &pcie->pdev->dev;
> > +       struct device_node *np = dev->of_node;
> > +       resource_size_t iobase;
> > +       struct resource_entry *win;
> > +
> > +       err = of_pci_get_host_bridge_resources(np, 0, 0xff, &pcie->resources,
> > +                                              &iobase);
>
> Nit: I do not see the point of passing iobase. You do not support IO space,
> if there is an IO address space in DT you want the function to barf,
> you do not want the function to initialize iobase silently.
>
> So, you should pass iobase as NULL, if there is an IO space in DT:
>
> of_pci_get_host_bridge_resources()
>
> will barf and that's what you want to flag FW misconfigurations up.
Okay, will pass in NULL for iobase.
>
> [...]
>
> > +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?

Thanks for reviewing.

Regards
Ley Foon
--
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



[Index of Archives]     [DMA Engine]     [Linux Coverity]     [Linux USB]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Greybus]

  Powered by Linux