lkml.org 
[lkml]   [2015]   [Jan]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [PATCH 3/5] PCI: st: Provide support for the sti PCIe controller
    From
    Hi Arnd,

    On 17 December 2014 at 23:14, Arnd Bergmann <arnd@arndb.de> wrote:
    > On Wednesday 17 December 2014 11:34:44 Gabriel FERNANDEZ wrote:
    >> sti pcie is built around a Synopsis Designware PCIe IP.
    >>
    >> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
    >> Signed-off-by: Gabriel Fernandez <gabriel.fernandez@linaro.org>
    >> ---
    >> drivers/pci/host/Kconfig | 5 +
    >> drivers/pci/host/Makefile | 1 +
    >> drivers/pci/host/pci-st.c | 713 ++++++++++++++++++++++++++++++++++++++++++++++
    >> 3 files changed, 719 insertions(+)
    >> create mode 100644 drivers/pci/host/pci-st.c
    >>
    >> diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
    >> index c4b6568..999d2b9 100644
    >> --- a/drivers/pci/host/Kconfig
    >> +++ b/drivers/pci/host/Kconfig
    >> @@ -102,4 +102,9 @@ config PCI_LAYERSCAPE
    >> help
    >> Say Y here if you want PCIe controller support on Layerscape SoCs.
    >>
    >> +config PCI_ST
    >> + bool "ST STiH41x PCIe controller"
    >> + depends on ARCH_STI
    >> + select PCIE_DW
    >
    > I'd use 'depends on ARCH_STI || (ARM && COMPILE_TEST)' to enable
    > building this on other platforms for test purposes.
    >
    ok

    >> +
    >> +#define to_st_pcie(x) container_of(x, struct st_pcie, pp)
    >> +
    >> +/**
    >> + * struct st_pcie_ops - SOC dependent data
    >> + * @init: reference to controller power & reset init routine
    >> + * @enable_ltssm: reference to controller link enable routine
    >> + * @disable_ltssm: reference to controller link disable routine
    >> + * @phy_auto: flag when phy automatically configured
    >> + */
    >> +struct st_pcie_ops {
    >> + int (*init)(struct pcie_port *pp);
    >> + int (*enable_ltssm)(struct pcie_port *pp);
    >> + int (*disable_ltssm)(struct pcie_port *pp);
    >> + bool phy_auto;
    >> +};
    >
    ok, i will fix it.

    > It would be better not to invent another level of indirection. Try
    > turning this around so you have a driver that binds to the specific
    > SoC compatible string for the PCIe port while calling into a common
    > library module for things that are shared.
    >
    >> +/*
    >> + * On ARM platforms, we actually get a bus error returned when the PCIe IP
    >> + * returns a UR or CRS instead of an OK.
    >> + */
    >> +static int st_pcie_abort_handler(unsigned long addr, unsigned int fsr,
    >> + struct pt_regs *regs)
    >> +{
    >> + return 0;
    >> +}
    >
    > You should check that it's actually PCI that caused the abort. Don't
    > just ignore a hard error condition.
    >
    > Usually there are registers in the PCI core that let you identify what
    > happened.
    >
    We return 0 because abort handler is not activated during boot.

    >> +static int st_pcie_rd_other_conf(struct pcie_port *pp, struct pci_bus *bus,
    >> + unsigned int devfn, int where, int size,
    >> + u32 *val)
    >> +{
    >> + u32 data;
    >> + u32 bdf;
    >> + struct st_pcie *pcie = to_st_pcie(pp);
    >> + int is_root_bus = pci_is_root_bus(bus);
    >> + int retry_count = 0;
    >> + int ret;
    >> + void __iomem *addr;
    >> +
    >> + /*
    >> + * Prerequisite
    >> + * PCI express devices will respond to all config type 0 cycles, since
    >> + * they are point to point links. Thus to avoid probing for multiple
    >> + * devices on the root, dw-pcie already check for us if it is on the
    >> + * root bus / other slots. Also, dw-pcie checks for the link being up
    >> + * as we will hang if we issue a config request and the link is down.
    >> + * A switch will reject requests for slots it knows do not exist.
    >> + */
    >> + bdf = bdf_num(bus->number, devfn, is_root_bus);
    >> + addr = pcie->config_area + config_addr(where,
    >> + bus->parent->number == pp->root_bus_nr);
    >> +retry:
    >> + /* Set the config packet devfn */
    >> + writel_relaxed(bdf, pp->dbi_base + FUNC0_BDF_NUM);
    >> + readl_relaxed(pp->dbi_base + FUNC0_BDF_NUM);
    >> +
    >> + ret = dw_pcie_cfg_read(addr, where, size, &data);
    >> +
    >> + /*
    >> + * This is intended to help with when we are probing the bus. The
    >> + * problem is that the wrapper logic doesn't have any way to
    >> + * interrogate if the configuration request failed or not.
    >> + * On the ARM we actually get a real bus error.
    >> + *
    >> + * Unfortunately this means it is impossible to tell the difference
    >> + * between when a device doesn't exist (the switch will return a UR
    >> + * completion) or the device does exist but isn't yet ready to accept
    >> + * configuration requests (the device will return a CRS completion)
    >> + *
    >> + * The result of this is that we will miss devices when probing.
    >> + *
    >> + * So if we are trying to read the dev/vendor id on devfn 0 and we
    >> + * appear to get zero back, then we retry the request. We know that
    >> + * zero can never be a valid device/vendor id. The specification says
    >> + * we must retry for up to a second before we decide the device is
    >> + * dead. If we are still dead then we assume there is nothing there and
    >> + * return ~0
    >> + *
    >> + * The downside of this is that we incur a delay of 1s for every pci
    >> + * express link that doesn't have a device connected.
    >> + */
    >> + if (((where & ~3) == 0) && devfn == 0 && (data == 0 || data == ~0)) {
    >> + if (retry_count++ < 1000) {
    >> + mdelay(1);
    >> + goto retry;
    >> + } else {
    >> + *val = ~0;
    >> + return PCIBIOS_DEVICE_NOT_FOUND;
    >> + }
    >> + }
    >> +
    >> + *val = data;
    >> + return ret;
    >> +}
    >
    > A busy-loop is extremely nasty. If this is only during the initial bus
    > scan, could you use an msleep instead?
    >
    yes it's during the initial bus scan.
    But we can't use msleep because we are under raw_spin_lock_irqsave()
    see PCI_OP_READ() macro in drivers/pci/access.c

    > Also, it sounds like the error you get is actually the fault that you
    > are catching above. If this is correct, then use the fault handler to
    > communicate this to the probe function.
    >
    Same as above the handler is not activated during the boot and initial bus scan.


    >> +
    >> +static void st_pcie_board_reset(struct pcie_port *pp)
    >> +{
    >> + struct st_pcie *pcie = to_st_pcie(pp);
    >> +
    >> + if (!gpio_is_valid(pcie->reset_gpio))
    >> + return;
    >> +
    >> + if (gpio_direction_output(pcie->reset_gpio, 0)) {
    >> + dev_err(pp->dev, "Cannot set PERST# (gpio %u) to output\n",
    >> + pcie->reset_gpio);
    >> + return;
    >> + }
    >> +
    >> + /* From PCIe spec */
    >> + usleep_range(1000, 2000);
    >> + gpio_direction_output(pcie->reset_gpio, 1);
    >> +
    >> + /*
    >> + * PCIe specification states that you should not issue any config
    >> + * requests until 100ms after asserting reset, so we enforce that here
    >> + */
    >> + usleep_range(100000, 150000);
    >> +}
    >
    > This seems hardly performance critical, just use msleep(2) and
    > msleep(100) instead of the usleep_range().
    >

    ok i will fix it.


    >> +static void st_msi_init_one(struct pcie_port *pp)
    >> +{
    >> + struct st_pcie *pcie = to_st_pcie(pp);
    >> +
    >> + /*
    >> + * Set the magic address the hardware responds to. This has to be in
    >> + * the range the PCI controller can write to.
    >> + */
    >> + dw_pcie_msi_init(pp);
    >> +
    >> + if ((virt_to_phys((void *)pp->msi_data) < pcie->lmi->start) ||
    >> + (virt_to_phys((void *)pp->msi_data) > pcie->lmi->end))
    >> + dev_err(pp->dev, "MSI addr miss-configured\n");
    >> +}
    >
    > Why do you call virt_to_phys() here? Isn't msi_data a physical address?
    >
    msi_data is a virtual address, it's obtained through a
    __get_free_pages() function in dw_pcie_msi_init() procedure.

    >> +static int __init st_pcie_probe(struct platform_device *pdev)
    >
    > I'd suggest removing the __init here, as discussed in the review for
    > the qualcomm driver.
    >
    ok

    > Arnd

    BR

    Gabriel.


    \
     
     \ /
      Last update: 2015-01-19 14:21    [W:4.196 / U:0.064 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site