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

[Ley Foon Tan]

On Tue, Sep 8, 2015 at 5:19 PM, Lorenzo Pieralisi
<lorenzo.pieralisi@xxxxxxx> wrote:
> 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.
Actually, we don't have any calculation to choose the value. Just want
to give small relaxation between each read.
It is request from Marc in previous review.
>
>> 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.
Okay.
>
[...]
>> >
>> > > +
>> > > +       /* 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 for explanation. I will add it.

Thanks.

Regards
Ley Foon
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/