On Friday, October 21, 2016 9:45:36 AM CEST Ruqiang Ju wrote:
> Add PCIe controller drvier for HiSilicon STB SoCs,
> the controller is based on the DesignWare's PCIe core.
>
> Signed-off-by: Ruqiang Ju <juruqiang@xxxxxxxxxx>
> ---
> .../bindings/pci/hisilicon-histb-pcie.txt | 66 +++
> drivers/pci/host/Kconfig | 8 +
> drivers/pci/host/Makefile | 1 +
> drivers/pci/host/pcie-histb.c | 448 +++++++++++++++++++++
> 4 files changed, 523 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/pci/hisilicon-histb-pcie.txt
> create mode 100644 drivers/pci/host/pcie-histb.c
>
> diff --git a/Documentation/devicetree/bindings/pci/hisilicon-histb-pcie.txt b/Documentation/devicetree/bindings/pci/hisilicon-histb-pcie.txt
> new file mode 100644
> index 0000000..952f1db
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pci/hisilicon-histb-pcie.txt
> @@ -0,0 +1,66 @@
> +HiSilicon STB PCIe host bridge DT description
> +
> +HiSilicon PCIe host controller is based on Designware PCI core.
> +It shares common functions with PCIe Designware core driver and inherits
> +common properties defined in
> +Documentation/devicetree/bindings/pci/designware-pci.txt.
> +
> +Additional properties are described here:
> +
> +Required properties
> +- compatible: Should be one of the following strings
> + "hisilicon,histb-pcie",
> + "hisilicon,hi3798cv200-pcie"
> +- reg: Should contain sysctl, rc_dbi, config registers location and length.
> +- reg-names: Must include the following entries:
> + "sysctrl": system control registers of PCIe controller;
> + "rc_dbi": configuration space of PCIe controller;
Please use "-" instead of "_" in identifiers.
> + "config": configuration transaction space of PCIe controller.
> +- interrupts: MSI interrupt.
> +- interrupt-names: Must include "msi" entries.
Shouldn't this be handled using the msi-map property?
> +- clocks: List of phandle and clock specifier pairs as listed
> + in clock-names property.
> +- clock-name: Must include the following entries:
> + "aux_clk": auxiliary gate clock;
> + "pipe_clk": pipe gate clock;
> + "sys_clk": sys gate clock;
> + "bus_clk": bus gate clock.
drop the redundant _clk postfix.
> +- resets: List of phandle and reset specifier pairs as listed
> + in reset-names property
> +- reset-names: Must include the following entries:
> + "soft_reset": soft reset;
> + "sys_reset": sys reset;
> + "bus_rest": bus reset.
drop the _rest and _reset postfix.
> +Optional properties:
> +- power-gpios: pcie device power control gpio if needed;
> +- power-gpios-active-high: must include this propty
> + if active level is high.
> +- status: Either "ok" or "disabled".
> +
> +Example:
> + pcie@f9860000 {
> + compatible = "hisilicon,histb-pcie", "snps,dw-pcie";
> + reg = <0xf9860000 0x1000>,
> + <0xf0000000 0x2000>,
> + <0xf2000000 0x01000000>;
> + reg-names = "sysctrl", "rc_dbi", "config";
Could it be that the "sysctrl" is not actually part of the PCIe
block but instead part of the system controller block?
Please use a syscon reference instead for those.
> + #address-cells = <3>;
> + #size-cells = <2>;
> + device_type = "pci";
> + num-lanes = <1>;
> + ranges=<0x81000000 0 0 0xf4000000 0 0x00010000
> + 0x82000000 0 0xf3000000 0xf3000000 0 0x01000000>;
That is very short for the memory window. Is there no
prefetchable 64-bit window in the hardware?
> + /* check if the link is up or not */
> + while (!dw_pcie_link_up(pp)) {
> + mdelay(10);
> + count++;
> + if (count == 50) {
> + dev_err(pp->dev, "PCIe Link Fail\n");
> + return -EINVAL;
> + }
> + }
> +
That is a very long delay here. Please don't do that.
> + dev_info(pp->dev, "Link up\n");
> +
> + return 0;
> +}
> +
> +static void histb_pcie_host_init(struct pcie_port *pp)
> +{
> + histb_pcie_establish_link(pp);
> +
> + if (IS_ENABLED(CONFIG_PCI_MSI))
> + dw_pcie_msi_init(pp);
> +}
Just make it depen on PCI_MSI_IRQ_DOMAIN and drop the IS_ENABLED
check.
> +#ifdef CONFIG_PCI_MSI
> +static irqreturn_t histb_pcie_msi_irq_handler(int irq, void *arg)
> +{
> + struct pcie_port *pp = arg;
> +
> + return dw_handle_msi_irq(pp);
> +}
> +#endif
This seems misplaced here. How do the other dw-pcie drivers handle
it? If there is a chance that this driver gets reused on a future
product that has a GICv3, please write the driver so it can already
use that.
Arnd
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
