Messages in this thread | | | Date | Tue, 10 Dec 2019 17:49:51 -0600 | From | Bjorn Helgaas <> | Subject | Re: [PATCH v10 2/3] PCI: dwc: intel: PCIe RC controller driver |
| |
On Fri, Dec 06, 2019 at 03:27:49PM +0800, Dilip Kota wrote: > Add support to PCIe RC controller on Intel Gateway SoCs. > PCIe controller is based of Synopsys DesignWare PCIe core. > > Intel PCIe driver requires Upconfigure support, Fast Training > Sequence and link speed configurations. So adding the respective > helper functions in the PCIe DesignWare framework. > It also programs hardware autonomous speed during speed > configuration so defining it in pci_regs.h. > > Also, mark Intel PCIe driver depends on MSI IRQ Domain > as Synopsys DesignWare framework depends on the > PCI_MSI_IRQ_DOMAIN. > > Signed-off-by: Dilip Kota <eswara.kota@linux.intel.com> > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > Reviewed-by: Andrew Murray <andrew.murray@arm.com> > Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com> > Acked-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
> --- a/drivers/pci/controller/dwc/pcie-designware.c > +++ b/drivers/pci/controller/dwc/pcie-designware.c > @@ -14,6 +14,8 @@ > > #include "pcie-designware.h" > > +extern const unsigned char pcie_link_speed[];
This shouldn't be needed; there's a declaration in drivers/pci/pci.h.
> +struct intel_pcie_soc { > + unsigned int pcie_ver; > + unsigned int pcie_atu_offset; > + u32 num_viewport; > +};
Looks a little strange to have the fields below lined up but the ones above not.
> +struct intel_pcie_port { > + struct dw_pcie pci; > + void __iomem *app_base; > + struct gpio_desc *reset_gpio; > + u32 rst_intrvl; > + u32 max_speed; > + u32 link_gen; > + u32 max_width; > + u32 n_fts; > + struct clk *core_clk; > + struct reset_control *core_rst; > + struct phy *phy; > + u8 pcie_cap_ofst; > +}; > + > +static void pcie_update_bits(void __iomem *base, u32 ofs, u32 mask, u32 val) > +{ > + u32 old; > + > + old = readl(base + ofs); > + val = (old & ~mask) | (val & mask); > + > + if (val != old) > + writel(val, base + ofs);
I assume this is never used on registers where the "old & ~mask" part contains RW1C bits? If there are RW1C bits in that part, this will corrupt them.
> + if (!lpp->pcie_cap_ofst) { > + ret = dw_pcie_find_capability(&lpp->pci, PCI_CAP_ID_EXP); > + if (!ret) { > + ret = -ENXIO; > + dev_err(dev, "Invalid PCIe capability offset\n");
Some of your messages start with a capital letter, others not.
> +int intel_pcie_msi_init(struct pcie_port *pp)
You might add a comment here like the one at ks_pcie_am654_msi_host_init(). Since the users of the .msi_host_init() function pointer only call the function if the pointer is non-NULL, it's not completely obvious why you have this stub function.
> +{ > + /* PCIe MSI/MSIx is handled by MSI in x86 processor */ > + return 0; > +}
> + /* > + * Intel PCIe doesn't configure IO region, so set viewport > + * to not to perform IO region access.
s/to not to/to not/
> + */ > + pci->num_viewport = data->num_viewport; > + > + dev_info(dev, "Intel PCIe Root Complex Port init done\n");
Probably superfluous.
> + > + return ret;
Since the return value is known here:
return 0;
> +}
| |