linux-pci.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 0/2] PCI: Add new Unisoc PCIe driver
@ 2020-09-09  9:48 Hongtao Wu
  2020-09-09  9:48 ` [PATCH v3 1/2] dt-bindings: PCI: sprd: Document Unisoc PCIe RC host controller Hongtao Wu
  2020-09-09  9:48 ` [PATCH v3 2/2] PCI: sprd: Add support for Unisoc SoCs' PCIe controller Hongtao Wu
  0 siblings, 2 replies; 7+ messages in thread
From: Hongtao Wu @ 2020-09-09  9:48 UTC (permalink / raw)
  To: Lorenzo Pieralisi, Rob Herring
  Cc: Orson Zhai, Baolin Wang, Chunyan Zhang, linux-pci, devicetree,
	linux-kernel, Hongtao Wu

From: Hongtao Wu <billows.wu@unisoc.com>

This series adds PCIe controller driver for Unisoc SoCs.
This controller is based on DesignWare PCIe IP.

Changes from v1:
1) Test this patch on top of Rob Herring's 40 part series of DWC clean-ups:
   https://lore.kernel.org/linux-pci/20200821035420.380495-1-robh@kernel.org/

2) Delete empty function

3) Document property "sprd,pcie-poweron-syscons" and
   'sprd,pcie-poweroff-syscons'

4) Delete runtime suspend/resume function

5) Add COMPILE_TEST which CONFIG_PCIE_SPRD depends on

Changes from v2:
1) Change RC mode to host mode in drivers/pci/controller/dwc/Kconfig

2) Change Signed-off-by from Billows Wu to Hongtao Wu

Hongtao Wu (2):
  dt-bindings: PCI: sprd: Document Unisoc PCIe RC host controller
  PCI: sprd: Add support for Unisoc SoCs' PCIe controller

 .../devicetree/bindings/pci/sprd-pcie.yaml         | 101 +++++++++
 drivers/pci/controller/dwc/Kconfig                 |  13 ++
 drivers/pci/controller/dwc/Makefile                |   1 +
 drivers/pci/controller/dwc/pcie-sprd.c             | 231 +++++++++++++++++++++
 4 files changed, 346 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/sprd-pcie.yaml
 create mode 100644 drivers/pci/controller/dwc/pcie-sprd.c

--
2.7.4


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH v3 1/2] dt-bindings: PCI: sprd: Document Unisoc PCIe RC host controller
  2020-09-09  9:48 [PATCH v3 0/2] PCI: Add new Unisoc PCIe driver Hongtao Wu
@ 2020-09-09  9:48 ` Hongtao Wu
  2020-09-15 17:25   ` Rob Herring
  2020-09-09  9:48 ` [PATCH v3 2/2] PCI: sprd: Add support for Unisoc SoCs' PCIe controller Hongtao Wu
  1 sibling, 1 reply; 7+ messages in thread
From: Hongtao Wu @ 2020-09-09  9:48 UTC (permalink / raw)
  To: Lorenzo Pieralisi, Rob Herring
  Cc: Orson Zhai, Baolin Wang, Chunyan Zhang, linux-pci, devicetree,
	linux-kernel, Hongtao Wu

From: Hongtao Wu <billows.wu@unisoc.com>

This series adds PCIe bindings for Unisoc SoCs.
This controller is based on DesignWare PCIe IP.

Signed-off-by: Hongtao Wu <billows.wu@unisoc.com>
---
 .../devicetree/bindings/pci/sprd-pcie.yaml         | 101 +++++++++++++++++++++
 1 file changed, 101 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/sprd-pcie.yaml

diff --git a/Documentation/devicetree/bindings/pci/sprd-pcie.yaml b/Documentation/devicetree/bindings/pci/sprd-pcie.yaml
new file mode 100644
index 0000000..c52edfb
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/sprd-pcie.yaml
@@ -0,0 +1,101 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pci/sprd-pcie.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: SoC PCIe Host Controller Device Tree Bindings
+
+maintainers:
+  - Hongtao Wu <billows.wu@unisoc.com>
+
+allOf:
+  - $ref: /schemas/pci/pci-bus.yaml#
+
+properties:
+  compatible:
+    items:
+      - const: sprd,pcie-rc
+
+  reg:
+    minItems: 2
+    items:
+      - description: Controller control and status registers.
+      - description: PCIe configuration registers.
+
+  reg-names:
+    items:
+      - const: dbi
+      - const: config
+
+  ranges:
+    maxItems: 2
+
+  num-lanes:
+    maximum: 1
+    description: Number of lanes to use for this port.
+
+  interrupts:
+    minItems: 1
+    description: Builtin MSI controller and PCIe host controller.
+
+  interrupt-names:
+    items:
+      - const: msi
+
+  sprd-pcie-poweron-syscons:
+    minItems: 1
+    description: Global register.
+      The first value is the phandle to the global registers required to
+      confige PCIe phy, clock and so on.
+      The second value is the global register type which indicates whether it
+      is a set/clear register or not.
+      The third value is the time to delay after the global register is set or
+      cleared.
+      The fourth value is the global register address.
+      The fifth value is the the mask value that the global register must
+      be operate.
+      The sixth value is the value that will be set to the global register.
+      Note that Some Unisoc global registers have not been upstreamed.
+      The global register and its mask can't be found in linux kernel,
+      so we use an offset address and a number to instead them.
+
+required:
+  - compatible
+  - reg
+  - reg-names
+  - num-lanes
+  - ranges
+  - interrupts
+  - interrupt-names
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+
+    ipa {
+        #address-cells = <2>;
+        #size-cells = <2>;
+
+        pcie0: pcie@2b100000 {
+            compatible = "sprd,pcie-rc";
+            reg = <0x0 0x2b100000 0x0 0x2000>,
+                  <0x2 0x00000000 0x0 0x2000>;
+            reg-names = "dbi", "config";
+            #address-cells = <3>;
+            #size-cells = <2>;
+            device_type = "pci";
+            ranges = <0x01000000 0x0 0x00000000 0x2 0x00002000 0x0 0x00010000>,
+                     <0x03000000 0x0 0x10000000 0x2 0x10000000 0x1 0xefffffff>;
+            num-lanes = <1>;
+            interrupts = <GIC_SPI 153 IRQ_TYPE_LEVEL_HIGH>;
+            interrupt-names = "msi";
+
+            sprd,pcie-poweron-syscons =
+                <&ap_ipa_ahb_regs 0 0 0x0000 0x40 0x40>,
+                <&ap_ipa_ahb_regs 0 0 0x0000 0x20 0x20>;
+            sprd,pcie-poweroff-syscons =
+                <&ap_ipa_ahb_regs 0 0 0x0000 0x20 0x0>,
+                <&ap_ipa_ahb_regs 0 0 0x0000 0x40 0x0>;
+        };
+    };
--
2.7.4


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH v3 2/2] PCI: sprd: Add support for Unisoc SoCs' PCIe controller
  2020-09-09  9:48 [PATCH v3 0/2] PCI: Add new Unisoc PCIe driver Hongtao Wu
  2020-09-09  9:48 ` [PATCH v3 1/2] dt-bindings: PCI: sprd: Document Unisoc PCIe RC host controller Hongtao Wu
@ 2020-09-09  9:48 ` Hongtao Wu
  2020-09-15 17:31   ` Rob Herring
  1 sibling, 1 reply; 7+ messages in thread
From: Hongtao Wu @ 2020-09-09  9:48 UTC (permalink / raw)
  To: Lorenzo Pieralisi, Rob Herring
  Cc: Orson Zhai, Baolin Wang, Chunyan Zhang, linux-pci, devicetree,
	linux-kernel, Hongtao Wu

From: Hongtao Wu <billows.wu@unisoc.com>

This series adds PCIe controller driver for Unisoc SoCs.
This controller is based on DesignWare PCIe IP.

Signed-off-by: Hongtao Wu <billows.wu@unisoc.com>
---
 drivers/pci/controller/dwc/Kconfig     |  13 ++
 drivers/pci/controller/dwc/Makefile    |   1 +
 drivers/pci/controller/dwc/pcie-sprd.c | 231 +++++++++++++++++++++++++++++++++
 3 files changed, 245 insertions(+)
 create mode 100644 drivers/pci/controller/dwc/pcie-sprd.c

diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
index 044a376..0553010 100644
--- a/drivers/pci/controller/dwc/Kconfig
+++ b/drivers/pci/controller/dwc/Kconfig
@@ -311,4 +311,17 @@ config PCIE_AL
 	  required only for DT-based platforms. ACPI platforms with the
 	  Annapurna Labs PCIe controller don't need to enable this.

+
+config PCIE_SPRD
+	tristate "Unisoc PCIe controller - Host Mode"
+	depends on ARCH_SPRD || COMPILE_TEST
+	depends on PCI_MSI_IRQ_DOMAIN
+	select PCIE_DW_HOST
+	help
+	  Unisoc PCIe controller uses the DesignWare core. It can be configured
+	  as an Endpoint (EP) or a Root complex (RC). In order to enable host
+	  mode (the controller works as RC), PCIE_SPRD must be selected.
+	  Say Y or M here if you want to PCIe RC controller support on Unisoc
+	  SoCs.
+
 endmenu
diff --git a/drivers/pci/controller/dwc/Makefile b/drivers/pci/controller/dwc/Makefile
index a751553..eb546e9 100644
--- a/drivers/pci/controller/dwc/Makefile
+++ b/drivers/pci/controller/dwc/Makefile
@@ -20,6 +20,7 @@ obj-$(CONFIG_PCI_MESON) += pci-meson.o
 obj-$(CONFIG_PCIE_TEGRA194) += pcie-tegra194.o
 obj-$(CONFIG_PCIE_UNIPHIER) += pcie-uniphier.o
 obj-$(CONFIG_PCIE_UNIPHIER_EP) += pcie-uniphier-ep.o
+obj-$(CONFIG_PCIE_SPRD) += pcie-sprd.o

 # The following drivers are for devices that use the generic ACPI
 # pci_root.c driver but don't support standard ECAM config access.
diff --git a/drivers/pci/controller/dwc/pcie-sprd.c b/drivers/pci/controller/dwc/pcie-sprd.c
new file mode 100644
index 0000000..cec4f34
--- /dev/null
+++ b/drivers/pci/controller/dwc/pcie-sprd.c
@@ -0,0 +1,231 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * PCIe host controller driver for Unisoc SoCs
+ *
+ * Copyright (C) 2020 Unisoc, Inc.
+ *
+ * Author: Hongtao Wu <Billows.Wu@unisoc.com>
+ */
+
+#include <linux/delay.h>
+#include <linux/gpio/consumer.h>
+#include <linux/interrupt.h>
+#include <linux/mfd/syscon.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/of_irq.h>
+#include <linux/platform_device.h>
+#include <linux/property.h>
+#include <linux/regmap.h>
+
+#include "pcie-designware.h"
+
+#define NUM_OF_ARGS 5
+
+struct sprd_pcie {
+	struct dw_pcie pci;
+};
+
+static int sprd_pcie_syscon_setting(struct platform_device *pdev, char *env)
+{
+	struct device_node *np = pdev->dev.of_node;
+	int i, count, err;
+	u32 type, delay, reg, mask, val, tmp_val;
+	struct of_phandle_args out_args;
+	struct regmap *iomap;
+	struct device *dev = &pdev->dev;
+
+	if (!of_find_property(np, env, NULL)) {
+		dev_info(dev, "There isn't property %s in dts\n", env);
+		return 0;
+	}
+
+	count = of_property_count_elems_of_size(np, env,
+					(NUM_OF_ARGS + 1) * sizeof(u32));
+	dev_info(dev, "Property (%s) reg count is %d :\n", env, count);
+
+	for (i = 0; i < count; i++) {
+		err = of_parse_phandle_with_fixed_args(np, env, NUM_OF_ARGS,
+						       i, &out_args);
+		if (err < 0)
+			return err;
+
+		type = out_args.args[0];
+		delay = out_args.args[1];
+		reg = out_args.args[2];
+		mask = out_args.args[3];
+		val = out_args.args[4];
+
+		iomap = syscon_node_to_regmap(out_args.np);
+
+		switch (type) {
+		case 0:
+			regmap_update_bits(iomap, reg, mask, val);
+			break;
+
+		case 1:
+			regmap_read(iomap, reg, &tmp_val);
+			tmp_val &= (~mask);
+			tmp_val |= (val & mask);
+			regmap_write(iomap, reg, tmp_val);
+			break;
+		default:
+			break;
+		}
+
+		if (delay)
+			usleep_range(delay, delay + 10);
+
+		regmap_read(iomap, reg, &tmp_val);
+		dev_dbg(dev,
+			"%2d:reg[0x%8x] mask[0x%8x] val[0x%8x] result[0x%8x]\n",
+			i, reg, mask, val, tmp_val);
+	}
+
+	return i;
+}
+
+static int sprd_pcie_perst_assert(struct platform_device *pdev)
+{
+	return sprd_pcie_syscon_setting(pdev, "sprd,pcie-perst-assert");
+}
+
+static int sprd_pcie_perst_deassert(struct platform_device *pdev)
+{
+	return sprd_pcie_syscon_setting(pdev, "sprd,pcie-perst-deassert");
+}
+
+static int sprd_pcie_power_on(struct platform_device *pdev)
+{
+	int ret;
+	struct device *dev = &pdev->dev;
+
+	ret = sprd_pcie_syscon_setting(pdev, "sprd,pcie-poweron-syscons");
+	if (ret < 0)
+		dev_err(dev,
+			"failed to set pcie poweroff syscons, return %d\n",
+			ret);
+
+	sprd_pcie_perst_deassert(pdev);
+
+	return ret;
+}
+
+static int sprd_pcie_power_off(struct platform_device *pdev)
+{
+	int ret;
+	struct device *dev = &pdev->dev;
+
+	sprd_pcie_perst_assert(pdev);
+
+	ret = sprd_pcie_syscon_setting(pdev, "sprd,pcie-poweroff-syscons");
+	if (ret < 0)
+		dev_err(dev,
+			"failed to set pcie poweroff syscons, return %d\n",
+			ret);
+
+	return ret;
+}
+
+static int sprd_pcie_host_init(struct pcie_port *pp)
+{
+	int ret;
+	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
+
+	dw_pcie_setup_rc(pp);
+	dw_pcie_msi_init(pp);
+
+	ret = dw_pcie_wait_for_link(pci);
+	if (ret)
+		dev_err(pci->dev, "pcie ep may has not been powered on yet\n");
+
+	return ret;
+}
+
+static const struct dw_pcie_host_ops sprd_pcie_host_ops = {
+	.host_init = sprd_pcie_host_init,
+};
+
+static int sprd_add_pcie_port(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct sprd_pcie *ctrl = platform_get_drvdata(pdev);
+	struct dw_pcie *pci = &ctrl->pci;
+	struct pcie_port *pp = &pci->pp;
+
+	pci->dbi_base = devm_platform_ioremap_resource_byname(pdev, "dbi");
+	if (IS_ERR(pci->dbi_base)) {
+		dev_err(dev, "failed to get rc dbi base\n");
+		return PTR_ERR(pci->dbi_base);
+	}
+
+	pp->ops = &sprd_pcie_host_ops;
+
+	if (IS_ENABLED(CONFIG_PCI_MSI)) {
+		pp->msi_irq = platform_get_irq_byname(pdev, "msi");
+		if (pp->msi_irq < 0) {
+			dev_err(dev, "failed to get msi, return %d\n",
+				pp->msi_irq);
+			return pp->msi_irq;
+		}
+	}
+
+	return dw_pcie_host_init(pp);
+}
+
+static int sprd_pcie_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct sprd_pcie *ctrl;
+	struct dw_pcie *pci;
+	int ret;
+
+	ctrl = devm_kzalloc(dev, sizeof(*ctrl), GFP_KERNEL);
+	if (!ctrl)
+		return -ENOMEM;
+
+	pci = &ctrl->pci;
+	pci->dev = dev;
+
+	platform_set_drvdata(pdev, ctrl);
+
+	ret = sprd_pcie_power_on(pdev);
+	if (ret < 0) {
+		dev_err(dev, "failed to get pcie poweron syscons, return %d\n",
+			ret);
+		goto err_power_off;
+	}
+
+	ret = sprd_add_pcie_port(pdev);
+	if (ret) {
+		dev_warn(dev, "failed to initialize RC controller\n");
+		return ret;
+	}
+
+	return 0;
+
+err_power_off:
+	sprd_pcie_power_off(pdev);
+
+	return ret;
+}
+
+static const struct of_device_id sprd_pcie_of_match[] = {
+	{
+		.compatible = "sprd,pcie-rc",
+	},
+	{},
+};
+
+static struct platform_driver sprd_pcie_driver = {
+	.probe = sprd_pcie_probe,
+	.driver = {
+		.name = "sprd-pcie",
+		.of_match_table = sprd_pcie_of_match,
+	},
+};
+
+module_platform_driver(sprd_pcie_driver);
+
+MODULE_DESCRIPTION("Unisoc PCIe host controller driver");
+MODULE_LICENSE("GPL v2");
--
2.7.4


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH v3 1/2] dt-bindings: PCI: sprd: Document Unisoc PCIe RC host controller
  2020-09-09  9:48 ` [PATCH v3 1/2] dt-bindings: PCI: sprd: Document Unisoc PCIe RC host controller Hongtao Wu
@ 2020-09-15 17:25   ` Rob Herring
  2020-09-18  3:18     ` Hongtao Wu
  0 siblings, 1 reply; 7+ messages in thread
From: Rob Herring @ 2020-09-15 17:25 UTC (permalink / raw)
  To: Hongtao Wu
  Cc: Lorenzo Pieralisi, Orson Zhai, Baolin Wang, Chunyan Zhang,
	linux-pci, devicetree, linux-kernel, Hongtao Wu

On Wed, Sep 09, 2020 at 05:48:31PM +0800, Hongtao Wu wrote:
> From: Hongtao Wu <billows.wu@unisoc.com>
> 
> This series adds PCIe bindings for Unisoc SoCs.
> This controller is based on DesignWare PCIe IP.
> 
> Signed-off-by: Hongtao Wu <billows.wu@unisoc.com>
> ---
>  .../devicetree/bindings/pci/sprd-pcie.yaml         | 101 +++++++++++++++++++++
>  1 file changed, 101 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pci/sprd-pcie.yaml
> 
> diff --git a/Documentation/devicetree/bindings/pci/sprd-pcie.yaml b/Documentation/devicetree/bindings/pci/sprd-pcie.yaml
> new file mode 100644
> index 0000000..c52edfb
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pci/sprd-pcie.yaml
> @@ -0,0 +1,101 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/pci/sprd-pcie.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: SoC PCIe Host Controller Device Tree Bindings
> +
> +maintainers:
> +  - Hongtao Wu <billows.wu@unisoc.com>
> +
> +allOf:
> +  - $ref: /schemas/pci/pci-bus.yaml#
> +
> +properties:
> +  compatible:
> +    items:
> +      - const: sprd,pcie-rc
> +
> +  reg:
> +    minItems: 2
> +    items:
> +      - description: Controller control and status registers.
> +      - description: PCIe configuration registers.
> +
> +  reg-names:
> +    items:
> +      - const: dbi
> +      - const: config
> +
> +  ranges:
> +    maxItems: 2
> +
> +  num-lanes:
> +    maximum: 1
> +    description: Number of lanes to use for this port.
> +
> +  interrupts:
> +    minItems: 1
> +    description: Builtin MSI controller and PCIe host controller.
> +
> +  interrupt-names:
> +    items:
> +      - const: msi
> +
> +  sprd-pcie-poweron-syscons:

Doesn't match the example.

> +    minItems: 1
> +    description: Global register.
> +      The first value is the phandle to the global registers required to
> +      confige PCIe phy, clock and so on.
> +      The second value is the global register type which indicates whether it
> +      is a set/clear register or not.
> +      The third value is the time to delay after the global register is set or
> +      cleared.
> +      The fourth value is the global register address.
> +      The fifth value is the the mask value that the global register must
> +      be operate.
> +      The sixth value is the value that will be set to the global register.
> +      Note that Some Unisoc global registers have not been upstreamed.
> +      The global register and its mask can't be found in linux kernel,
> +      so we use an offset address and a number to instead them.

From the example, it looks like you set/clear 2 bits for power on/off. 
What's the worst case you expect here? What do the 2 bits do? If they 
are for clocks, resets, or power domains, then we have bindings for 
those which should be used. This use of phandles to syscons should be 
avoided whenever possible.

If we wanted a language for specifying sequences of register accesses in 
DT, we would have defined that a long time ago.

> +
> +required:
> +  - compatible
> +  - reg
> +  - reg-names
> +  - num-lanes
> +  - ranges
> +  - interrupts
> +  - interrupt-names
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +
> +    ipa {
> +        #address-cells = <2>;
> +        #size-cells = <2>;
> +
> +        pcie0: pcie@2b100000 {
> +            compatible = "sprd,pcie-rc";
> +            reg = <0x0 0x2b100000 0x0 0x2000>,
> +                  <0x2 0x00000000 0x0 0x2000>;
> +            reg-names = "dbi", "config";
> +            #address-cells = <3>;
> +            #size-cells = <2>;
> +            device_type = "pci";
> +            ranges = <0x01000000 0x0 0x00000000 0x2 0x00002000 0x0 0x00010000>,
> +                     <0x03000000 0x0 0x10000000 0x2 0x10000000 0x1 0xefffffff>;
> +            num-lanes = <1>;
> +            interrupts = <GIC_SPI 153 IRQ_TYPE_LEVEL_HIGH>;
> +            interrupt-names = "msi";
> +
> +            sprd,pcie-poweron-syscons =
> +                <&ap_ipa_ahb_regs 0 0 0x0000 0x40 0x40>,
> +                <&ap_ipa_ahb_regs 0 0 0x0000 0x20 0x20>;
> +            sprd,pcie-poweroff-syscons =

Not documented.

> +                <&ap_ipa_ahb_regs 0 0 0x0000 0x20 0x0>,
> +                <&ap_ipa_ahb_regs 0 0 0x0000 0x40 0x0>;
> +        };
> +    };
> --
> 2.7.4
> 

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v3 2/2] PCI: sprd: Add support for Unisoc SoCs' PCIe controller
  2020-09-09  9:48 ` [PATCH v3 2/2] PCI: sprd: Add support for Unisoc SoCs' PCIe controller Hongtao Wu
@ 2020-09-15 17:31   ` Rob Herring
  2020-09-18  2:00     ` Hongtao Wu
  0 siblings, 1 reply; 7+ messages in thread
From: Rob Herring @ 2020-09-15 17:31 UTC (permalink / raw)
  To: Hongtao Wu
  Cc: Lorenzo Pieralisi, Orson Zhai, Baolin Wang, Chunyan Zhang,
	linux-pci, devicetree, linux-kernel, Hongtao Wu

On Wed, Sep 09, 2020 at 05:48:32PM +0800, Hongtao Wu wrote:
> From: Hongtao Wu <billows.wu@unisoc.com>
> 
> This series adds PCIe controller driver for Unisoc SoCs.
> This controller is based on DesignWare PCIe IP.
> 
> Signed-off-by: Hongtao Wu <billows.wu@unisoc.com>
> ---
>  drivers/pci/controller/dwc/Kconfig     |  13 ++
>  drivers/pci/controller/dwc/Makefile    |   1 +
>  drivers/pci/controller/dwc/pcie-sprd.c | 231 +++++++++++++++++++++++++++++++++
>  3 files changed, 245 insertions(+)
>  create mode 100644 drivers/pci/controller/dwc/pcie-sprd.c
> 
> diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
> index 044a376..0553010 100644
> --- a/drivers/pci/controller/dwc/Kconfig
> +++ b/drivers/pci/controller/dwc/Kconfig
> @@ -311,4 +311,17 @@ config PCIE_AL
>  	  required only for DT-based platforms. ACPI platforms with the
>  	  Annapurna Labs PCIe controller don't need to enable this.
> 
> +
> +config PCIE_SPRD
> +	tristate "Unisoc PCIe controller - Host Mode"
> +	depends on ARCH_SPRD || COMPILE_TEST
> +	depends on PCI_MSI_IRQ_DOMAIN
> +	select PCIE_DW_HOST
> +	help
> +	  Unisoc PCIe controller uses the DesignWare core. It can be configured
> +	  as an Endpoint (EP) or a Root complex (RC). In order to enable host
> +	  mode (the controller works as RC), PCIE_SPRD must be selected.
> +	  Say Y or M here if you want to PCIe RC controller support on Unisoc
> +	  SoCs.
> +
>  endmenu
> diff --git a/drivers/pci/controller/dwc/Makefile b/drivers/pci/controller/dwc/Makefile
> index a751553..eb546e9 100644
> --- a/drivers/pci/controller/dwc/Makefile
> +++ b/drivers/pci/controller/dwc/Makefile
> @@ -20,6 +20,7 @@ obj-$(CONFIG_PCI_MESON) += pci-meson.o
>  obj-$(CONFIG_PCIE_TEGRA194) += pcie-tegra194.o
>  obj-$(CONFIG_PCIE_UNIPHIER) += pcie-uniphier.o
>  obj-$(CONFIG_PCIE_UNIPHIER_EP) += pcie-uniphier-ep.o
> +obj-$(CONFIG_PCIE_SPRD) += pcie-sprd.o
> 
>  # The following drivers are for devices that use the generic ACPI
>  # pci_root.c driver but don't support standard ECAM config access.
> diff --git a/drivers/pci/controller/dwc/pcie-sprd.c b/drivers/pci/controller/dwc/pcie-sprd.c
> new file mode 100644
> index 0000000..cec4f34
> --- /dev/null
> +++ b/drivers/pci/controller/dwc/pcie-sprd.c
> @@ -0,0 +1,231 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * PCIe host controller driver for Unisoc SoCs
> + *
> + * Copyright (C) 2020 Unisoc, Inc.
> + *
> + * Author: Hongtao Wu <Billows.Wu@unisoc.com>
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/interrupt.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/of_irq.h>
> +#include <linux/platform_device.h>
> +#include <linux/property.h>
> +#include <linux/regmap.h>
> +
> +#include "pcie-designware.h"
> +
> +#define NUM_OF_ARGS 5
> +
> +struct sprd_pcie {
> +	struct dw_pcie pci;
> +};
> +
> +static int sprd_pcie_syscon_setting(struct platform_device *pdev, char *env)
> +{
> +	struct device_node *np = pdev->dev.of_node;
> +	int i, count, err;
> +	u32 type, delay, reg, mask, val, tmp_val;
> +	struct of_phandle_args out_args;
> +	struct regmap *iomap;
> +	struct device *dev = &pdev->dev;
> +
> +	if (!of_find_property(np, env, NULL)) {
> +		dev_info(dev, "There isn't property %s in dts\n", env);
> +		return 0;
> +	}
> +
> +	count = of_property_count_elems_of_size(np, env,
> +					(NUM_OF_ARGS + 1) * sizeof(u32));
> +	dev_info(dev, "Property (%s) reg count is %d :\n", env, count);
> +
> +	for (i = 0; i < count; i++) {
> +		err = of_parse_phandle_with_fixed_args(np, env, NUM_OF_ARGS,
> +						       i, &out_args);
> +		if (err < 0)
> +			return err;
> +
> +		type = out_args.args[0];
> +		delay = out_args.args[1];
> +		reg = out_args.args[2];
> +		mask = out_args.args[3];
> +		val = out_args.args[4];
> +
> +		iomap = syscon_node_to_regmap(out_args.np);
> +
> +		switch (type) {
> +		case 0:
> +			regmap_update_bits(iomap, reg, mask, val);
> +			break;
> +
> +		case 1:
> +			regmap_read(iomap, reg, &tmp_val);
> +			tmp_val &= (~mask);
> +			tmp_val |= (val & mask);
> +			regmap_write(iomap, reg, tmp_val);
> +			break;
> +		default:
> +			break;
> +		}
> +
> +		if (delay)
> +			usleep_range(delay, delay + 10);
> +
> +		regmap_read(iomap, reg, &tmp_val);
> +		dev_dbg(dev,
> +			"%2d:reg[0x%8x] mask[0x%8x] val[0x%8x] result[0x%8x]\n",
> +			i, reg, mask, val, tmp_val);
> +	}
> +
> +	return i;
> +}
> +
> +static int sprd_pcie_perst_assert(struct platform_device *pdev)
> +{
> +	return sprd_pcie_syscon_setting(pdev, "sprd,pcie-perst-assert");

Not documented. This should probably use the reset binding.

> +}
> +
> +static int sprd_pcie_perst_deassert(struct platform_device *pdev)
> +{
> +	return sprd_pcie_syscon_setting(pdev, "sprd,pcie-perst-deassert");
> +}
> +
> +static int sprd_pcie_power_on(struct platform_device *pdev)
> +{
> +	int ret;
> +	struct device *dev = &pdev->dev;
> +
> +	ret = sprd_pcie_syscon_setting(pdev, "sprd,pcie-poweron-syscons");
> +	if (ret < 0)
> +		dev_err(dev,
> +			"failed to set pcie poweroff syscons, return %d\n",
> +			ret);
> +
> +	sprd_pcie_perst_deassert(pdev);
> +
> +	return ret;
> +}
> +
> +static int sprd_pcie_power_off(struct platform_device *pdev)
> +{
> +	int ret;
> +	struct device *dev = &pdev->dev;
> +
> +	sprd_pcie_perst_assert(pdev);
> +
> +	ret = sprd_pcie_syscon_setting(pdev, "sprd,pcie-poweroff-syscons");
> +	if (ret < 0)
> +		dev_err(dev,
> +			"failed to set pcie poweroff syscons, return %d\n",
> +			ret);
> +
> +	return ret;
> +}
> +
> +static int sprd_pcie_host_init(struct pcie_port *pp)
> +{
> +	int ret;
> +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> +
> +	dw_pcie_setup_rc(pp);
> +	dw_pcie_msi_init(pp);
> +
> +	ret = dw_pcie_wait_for_link(pci);
> +	if (ret)
> +		dev_err(pci->dev, "pcie ep may has not been powered on yet\n");
> +
> +	return ret;
> +}
> +
> +static const struct dw_pcie_host_ops sprd_pcie_host_ops = {
> +	.host_init = sprd_pcie_host_init,
> +};
> +
> +static int sprd_add_pcie_port(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct sprd_pcie *ctrl = platform_get_drvdata(pdev);
> +	struct dw_pcie *pci = &ctrl->pci;
> +	struct pcie_port *pp = &pci->pp;
> +
> +	pci->dbi_base = devm_platform_ioremap_resource_byname(pdev, "dbi");
> +	if (IS_ERR(pci->dbi_base)) {
> +		dev_err(dev, "failed to get rc dbi base\n");
> +		return PTR_ERR(pci->dbi_base);
> +	}
> +
> +	pp->ops = &sprd_pcie_host_ops;
> +
> +	if (IS_ENABLED(CONFIG_PCI_MSI)) {

I don't think this check is needed. The DW core won't setup the MSI if 
not enabled, so doesn't matter if msi_irq is initialized.

> +		pp->msi_irq = platform_get_irq_byname(pdev, "msi");
> +		if (pp->msi_irq < 0) {
> +			dev_err(dev, "failed to get msi, return %d\n",
> +				pp->msi_irq);

No need to print an error, the core does this.

> +			return pp->msi_irq;
> +		}
> +	}
> +
> +	return dw_pcie_host_init(pp);
> +}
> +
> +static int sprd_pcie_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct sprd_pcie *ctrl;
> +	struct dw_pcie *pci;
> +	int ret;
> +
> +	ctrl = devm_kzalloc(dev, sizeof(*ctrl), GFP_KERNEL);
> +	if (!ctrl)
> +		return -ENOMEM;
> +
> +	pci = &ctrl->pci;
> +	pci->dev = dev;
> +
> +	platform_set_drvdata(pdev, ctrl);
> +
> +	ret = sprd_pcie_power_on(pdev);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to get pcie poweron syscons, return %d\n",
> +			ret);
> +		goto err_power_off;
> +	}
> +
> +	ret = sprd_add_pcie_port(pdev);
> +	if (ret) {
> +		dev_warn(dev, "failed to initialize RC controller\n");
> +		return ret;
> +	}
> +
> +	return 0;
> +
> +err_power_off:
> +	sprd_pcie_power_off(pdev);
> +
> +	return ret;
> +}
> +
> +static const struct of_device_id sprd_pcie_of_match[] = {
> +	{
> +		.compatible = "sprd,pcie-rc",
> +	},
> +	{},
> +};
> +
> +static struct platform_driver sprd_pcie_driver = {
> +	.probe = sprd_pcie_probe,

You need a .remove hook.

> +	.driver = {
> +		.name = "sprd-pcie",
> +		.of_match_table = sprd_pcie_of_match,
> +	},
> +};
> +
> +module_platform_driver(sprd_pcie_driver);
> +
> +MODULE_DESCRIPTION("Unisoc PCIe host controller driver");
> +MODULE_LICENSE("GPL v2");
> --
> 2.7.4
> 

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v3 2/2] PCI: sprd: Add support for Unisoc SoCs' PCIe controller
  2020-09-15 17:31   ` Rob Herring
@ 2020-09-18  2:00     ` Hongtao Wu
  0 siblings, 0 replies; 7+ messages in thread
From: Hongtao Wu @ 2020-09-18  2:00 UTC (permalink / raw)
  To: Rob Herring
  Cc: Lorenzo Pieralisi, Orson Zhai, Baolin Wang, Chunyan Zhang,
	linux-pci, devicetree, linux-kernel, Hongtao Wu

Hi Bob,
Thanks for the review.

On Wed, Sep 16, 2020 at 1:31 AM Rob Herring <robh@kernel.org> wrote:
>
> On Wed, Sep 09, 2020 at 05:48:32PM +0800, Hongtao Wu wrote:
> > From: Hongtao Wu <billows.wu@unisoc.com>
> >
> > This series adds PCIe controller driver for Unisoc SoCs.
> > This controller is based on DesignWare PCIe IP.
> >
> > Signed-off-by: Hongtao Wu <billows.wu@unisoc.com>
> > ---
> >  drivers/pci/controller/dwc/Kconfig     |  13 ++
> >  drivers/pci/controller/dwc/Makefile    |   1 +
> >  drivers/pci/controller/dwc/pcie-sprd.c | 231 +++++++++++++++++++++++++++++++++
> >  3 files changed, 245 insertions(+)
> >  create mode 100644 drivers/pci/controller/dwc/pcie-sprd.c
> >
> > diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
> > index 044a376..0553010 100644
> > --- a/drivers/pci/controller/dwc/Kconfig
> > +++ b/drivers/pci/controller/dwc/Kconfig
> > @@ -311,4 +311,17 @@ config PCIE_AL
> >         required only for DT-based platforms. ACPI platforms with the
> >         Annapurna Labs PCIe controller don't need to enable this.
> >
> > +
> > +config PCIE_SPRD
> > +     tristate "Unisoc PCIe controller - Host Mode"
> > +     depends on ARCH_SPRD || COMPILE_TEST
> > +     depends on PCI_MSI_IRQ_DOMAIN
> > +     select PCIE_DW_HOST
> > +     help
> > +       Unisoc PCIe controller uses the DesignWare core. It can be configured
> > +       as an Endpoint (EP) or a Root complex (RC). In order to enable host
> > +       mode (the controller works as RC), PCIE_SPRD must be selected.
> > +       Say Y or M here if you want to PCIe RC controller support on Unisoc
> > +       SoCs.
> > +
> >  endmenu
> > diff --git a/drivers/pci/controller/dwc/Makefile b/drivers/pci/controller/dwc/Makefile
> > index a751553..eb546e9 100644
> > --- a/drivers/pci/controller/dwc/Makefile
> > +++ b/drivers/pci/controller/dwc/Makefile
> > @@ -20,6 +20,7 @@ obj-$(CONFIG_PCI_MESON) += pci-meson.o
> >  obj-$(CONFIG_PCIE_TEGRA194) += pcie-tegra194.o
> >  obj-$(CONFIG_PCIE_UNIPHIER) += pcie-uniphier.o
> >  obj-$(CONFIG_PCIE_UNIPHIER_EP) += pcie-uniphier-ep.o
> > +obj-$(CONFIG_PCIE_SPRD) += pcie-sprd.o
> >
> >  # The following drivers are for devices that use the generic ACPI
> >  # pci_root.c driver but don't support standard ECAM config access.
> > diff --git a/drivers/pci/controller/dwc/pcie-sprd.c b/drivers/pci/controller/dwc/pcie-sprd.c
> > new file mode 100644
> > index 0000000..cec4f34
> > --- /dev/null
> > +++ b/drivers/pci/controller/dwc/pcie-sprd.c
> > @@ -0,0 +1,231 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * PCIe host controller driver for Unisoc SoCs
> > + *
> > + * Copyright (C) 2020 Unisoc, Inc.
> > + *
> > + * Author: Hongtao Wu <Billows.Wu@unisoc.com>
> > + */
> > +
> > +#include <linux/delay.h>
> > +#include <linux/gpio/consumer.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/mfd/syscon.h>
> > +#include <linux/module.h>
> > +#include <linux/of_device.h>
> > +#include <linux/of_irq.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/property.h>
> > +#include <linux/regmap.h>
> > +
> > +#include "pcie-designware.h"
> > +
> > +#define NUM_OF_ARGS 5
> > +
> > +struct sprd_pcie {
> > +     struct dw_pcie pci;
> > +};
> > +
> > +static int sprd_pcie_syscon_setting(struct platform_device *pdev, char *env)
> > +{
> > +     struct device_node *np = pdev->dev.of_node;
> > +     int i, count, err;
> > +     u32 type, delay, reg, mask, val, tmp_val;
> > +     struct of_phandle_args out_args;
> > +     struct regmap *iomap;
> > +     struct device *dev = &pdev->dev;
> > +
> > +     if (!of_find_property(np, env, NULL)) {
> > +             dev_info(dev, "There isn't property %s in dts\n", env);
> > +             return 0;
> > +     }
> > +
> > +     count = of_property_count_elems_of_size(np, env,
> > +                                     (NUM_OF_ARGS + 1) * sizeof(u32));
> > +     dev_info(dev, "Property (%s) reg count is %d :\n", env, count);
> > +
> > +     for (i = 0; i < count; i++) {
> > +             err = of_parse_phandle_with_fixed_args(np, env, NUM_OF_ARGS,
> > +                                                    i, &out_args);
> > +             if (err < 0)
> > +                     return err;
> > +
> > +             type = out_args.args[0];
> > +             delay = out_args.args[1];
> > +             reg = out_args.args[2];
> > +             mask = out_args.args[3];
> > +             val = out_args.args[4];
> > +
> > +             iomap = syscon_node_to_regmap(out_args.np);
> > +
> > +             switch (type) {
> > +             case 0:
> > +                     regmap_update_bits(iomap, reg, mask, val);
> > +                     break;
> > +
> > +             case 1:
> > +                     regmap_read(iomap, reg, &tmp_val);
> > +                     tmp_val &= (~mask);
> > +                     tmp_val |= (val & mask);
> > +                     regmap_write(iomap, reg, tmp_val);
> > +                     break;
> > +             default:
> > +                     break;
> > +             }
> > +
> > +             if (delay)
> > +                     usleep_range(delay, delay + 10);
> > +
> > +             regmap_read(iomap, reg, &tmp_val);
> > +             dev_dbg(dev,
> > +                     "%2d:reg[0x%8x] mask[0x%8x] val[0x%8x] result[0x%8x]\n",
> > +                     i, reg, mask, val, tmp_val);
> > +     }
> > +
> > +     return i;
> > +}
> > +
> > +static int sprd_pcie_perst_assert(struct platform_device *pdev)
> > +{
> > +     return sprd_pcie_syscon_setting(pdev, "sprd,pcie-perst-assert");
>
> Not documented. This should probably use the reset binding.
>

Thanks! I'll Document it in the next version.

> > +}
> > +
> > +static int sprd_pcie_perst_deassert(struct platform_device *pdev)
> > +{
> > +     return sprd_pcie_syscon_setting(pdev, "sprd,pcie-perst-deassert");
> > +}
> > +
> > +static int sprd_pcie_power_on(struct platform_device *pdev)
> > +{
> > +     int ret;
> > +     struct device *dev = &pdev->dev;
> > +
> > +     ret = sprd_pcie_syscon_setting(pdev, "sprd,pcie-poweron-syscons");
> > +     if (ret < 0)
> > +             dev_err(dev,
> > +                     "failed to set pcie poweroff syscons, return %d\n",
> > +                     ret);
> > +
> > +     sprd_pcie_perst_deassert(pdev);
> > +
> > +     return ret;
> > +}
> > +
> > +static int sprd_pcie_power_off(struct platform_device *pdev)
> > +{
> > +     int ret;
> > +     struct device *dev = &pdev->dev;
> > +
> > +     sprd_pcie_perst_assert(pdev);
> > +
> > +     ret = sprd_pcie_syscon_setting(pdev, "sprd,pcie-poweroff-syscons");
> > +     if (ret < 0)
> > +             dev_err(dev,
> > +                     "failed to set pcie poweroff syscons, return %d\n",
> > +                     ret);
> > +
> > +     return ret;
> > +}
> > +
> > +static int sprd_pcie_host_init(struct pcie_port *pp)
> > +{
> > +     int ret;
> > +     struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > +
> > +     dw_pcie_setup_rc(pp);
> > +     dw_pcie_msi_init(pp);
> > +
> > +     ret = dw_pcie_wait_for_link(pci);
> > +     if (ret)
> > +             dev_err(pci->dev, "pcie ep may has not been powered on yet\n");
> > +
> > +     return ret;
> > +}
> > +
> > +static const struct dw_pcie_host_ops sprd_pcie_host_ops = {
> > +     .host_init = sprd_pcie_host_init,
> > +};
> > +
> > +static int sprd_add_pcie_port(struct platform_device *pdev)
> > +{
> > +     struct device *dev = &pdev->dev;
> > +     struct sprd_pcie *ctrl = platform_get_drvdata(pdev);
> > +     struct dw_pcie *pci = &ctrl->pci;
> > +     struct pcie_port *pp = &pci->pp;
> > +
> > +     pci->dbi_base = devm_platform_ioremap_resource_byname(pdev, "dbi");
> > +     if (IS_ERR(pci->dbi_base)) {
> > +             dev_err(dev, "failed to get rc dbi base\n");
> > +             return PTR_ERR(pci->dbi_base);
> > +     }
> > +
> > +     pp->ops = &sprd_pcie_host_ops;
> > +
> > +     if (IS_ENABLED(CONFIG_PCI_MSI)) {
>
> I don't think this check is needed. The DW core won't setup the MSI if
> not enabled, so doesn't matter if msi_irq is initialized.
>

Thanks!
I'll delete it in the next version.

> > +             pp->msi_irq = platform_get_irq_byname(pdev, "msi");
> > +             if (pp->msi_irq < 0) {
> > +                     dev_err(dev, "failed to get msi, return %d\n",
> > +                             pp->msi_irq);
>
> No need to print an error, the core does this.
>

Thanks!
I'll delete it in the next version.

> > +                     return pp->msi_irq;
> > +             }
> > +     }
> > +
> > +     return dw_pcie_host_init(pp);
> > +}
> > +
> > +static int sprd_pcie_probe(struct platform_device *pdev)
> > +{
> > +     struct device *dev = &pdev->dev;
> > +     struct sprd_pcie *ctrl;
> > +     struct dw_pcie *pci;
> > +     int ret;
> > +
> > +     ctrl = devm_kzalloc(dev, sizeof(*ctrl), GFP_KERNEL);
> > +     if (!ctrl)
> > +             return -ENOMEM;
> > +
> > +     pci = &ctrl->pci;
> > +     pci->dev = dev;
> > +
> > +     platform_set_drvdata(pdev, ctrl);
> > +
> > +     ret = sprd_pcie_power_on(pdev);
> > +     if (ret < 0) {
> > +             dev_err(dev, "failed to get pcie poweron syscons, return %d\n",
> > +                     ret);
> > +             goto err_power_off;
> > +     }
> > +
> > +     ret = sprd_add_pcie_port(pdev);
> > +     if (ret) {
> > +             dev_warn(dev, "failed to initialize RC controller\n");
> > +             return ret;
> > +     }
> > +
> > +     return 0;
> > +
> > +err_power_off:
> > +     sprd_pcie_power_off(pdev);
> > +
> > +     return ret;
> > +}
> > +
> > +static const struct of_device_id sprd_pcie_of_match[] = {
> > +     {
> > +             .compatible = "sprd,pcie-rc",
> > +     },
> > +     {},
> > +};
> > +
> > +static struct platform_driver sprd_pcie_driver = {
> > +     .probe = sprd_pcie_probe,
>
> You need a .remove hook.
>

Yes! I'll fix it.

> > +     .driver = {
> > +             .name = "sprd-pcie",
> > +             .of_match_table = sprd_pcie_of_match,
> > +     },
> > +};
> > +
> > +module_platform_driver(sprd_pcie_driver);
> > +
> > +MODULE_DESCRIPTION("Unisoc PCIe host controller driver");
> > +MODULE_LICENSE("GPL v2");
> > --
> > 2.7.4
> >

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v3 1/2] dt-bindings: PCI: sprd: Document Unisoc PCIe RC host controller
  2020-09-15 17:25   ` Rob Herring
@ 2020-09-18  3:18     ` Hongtao Wu
  0 siblings, 0 replies; 7+ messages in thread
From: Hongtao Wu @ 2020-09-18  3:18 UTC (permalink / raw)
  To: Rob Herring
  Cc: Lorenzo Pieralisi, Orson Zhai, Baolin Wang, Chunyan Zhang,
	linux-pci, devicetree, linux-kernel, Hongtao Wu

On Wed, Sep 16, 2020 at 1:25 AM Rob Herring <robh@kernel.org> wrote:
>
> On Wed, Sep 09, 2020 at 05:48:31PM +0800, Hongtao Wu wrote:
> > From: Hongtao Wu <billows.wu@unisoc.com>
> >
> > This series adds PCIe bindings for Unisoc SoCs.
> > This controller is based on DesignWare PCIe IP.
> >
> > Signed-off-by: Hongtao Wu <billows.wu@unisoc.com>
> > ---
> >  .../devicetree/bindings/pci/sprd-pcie.yaml         | 101 +++++++++++++++++++++
> >  1 file changed, 101 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/pci/sprd-pcie.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/pci/sprd-pcie.yaml b/Documentation/devicetree/bindings/pci/sprd-pcie.yaml
> > new file mode 100644
> > index 0000000..c52edfb
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/pci/sprd-pcie.yaml
> > @@ -0,0 +1,101 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/pci/sprd-pcie.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: SoC PCIe Host Controller Device Tree Bindings
> > +
> > +maintainers:
> > +  - Hongtao Wu <billows.wu@unisoc.com>
> > +
> > +allOf:
> > +  - $ref: /schemas/pci/pci-bus.yaml#
> > +
> > +properties:
> > +  compatible:
> > +    items:
> > +      - const: sprd,pcie-rc
> > +
> > +  reg:
> > +    minItems: 2
> > +    items:
> > +      - description: Controller control and status registers.
> > +      - description: PCIe configuration registers.
> > +
> > +  reg-names:
> > +    items:
> > +      - const: dbi
> > +      - const: config
> > +
> > +  ranges:
> > +    maxItems: 2
> > +
> > +  num-lanes:
> > +    maximum: 1
> > +    description: Number of lanes to use for this port.
> > +
> > +  interrupts:
> > +    minItems: 1
> > +    description: Builtin MSI controller and PCIe host controller.
> > +
> > +  interrupt-names:
> > +    items:
> > +      - const: msi
> > +
> > +  sprd-pcie-poweron-syscons:
>

I am Sorry!
I'll fix it.

> Doesn't match the example.
>
> > +    minItems: 1
> > +    description: Global register.
> > +      The first value is the phandle to the global registers required to
> > +      confige PCIe phy, clock and so on.
> > +      The second value is the global register type which indicates whether it
> > +      is a set/clear register or not.
> > +      The third value is the time to delay after the global register is set or
> > +      cleared.
> > +      The fourth value is the global register address.
> > +      The fifth value is the the mask value that the global register must
> > +      be operate.
> > +      The sixth value is the value that will be set to the global register.
> > +      Note that Some Unisoc global registers have not been upstreamed.
> > +      The global register and its mask can't be found in linux kernel,
> > +      so we use an offset address and a number to instead them.
>
> From the example, it looks like you set/clear 2 bits for power on/off.
> What's the worst case you expect here? What do the 2 bits do? If they
> are for clocks, resets, or power domains, then we have bindings for
> those which should be used. This use of phandles to syscons should be
> avoided whenever possible.
>

There are two kinds of global register ( set/clear registers and
non-set/clear registers )
about PCIe on Unisoc SoCs.
Each set of set/clear registers contain two addresses. One can be
written and the other one
can be read. Different bits in  the set/clear register indicate
different functions, so we
set/clear one bit for power on/off.
The non-set/clear registers are normal which only have one address.

The second value in property 'sprd,pcie-poweron-syscons' is a flag
which indicates whether
the global register is set/clear or not. If this value is 1, we think
that it's a set/clear register.
If this value is 0, we think it's a non-set/clear register.

I wanted to parse all of the global registers about power on/off in an
array (include set/clear
registers and non-set/clear registers). However, it may not be a good idea.
I'll split the property 'sprd,pcie-poweron-syscons' info clocks, power
domains, phy and so on
in the next version.

> If we wanted a language for specifying sequences of register accesses in
> DT, we would have defined that a long time ago.
>

> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - reg-names
> > +  - num-lanes
> > +  - ranges
> > +  - interrupts
> > +  - interrupt-names
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> > +
> > +    ipa {
> > +        #address-cells = <2>;
> > +        #size-cells = <2>;
> > +
> > +        pcie0: pcie@2b100000 {
> > +            compatible = "sprd,pcie-rc";
> > +            reg = <0x0 0x2b100000 0x0 0x2000>,
> > +                  <0x2 0x00000000 0x0 0x2000>;
> > +            reg-names = "dbi", "config";
> > +            #address-cells = <3>;
> > +            #size-cells = <2>;
> > +            device_type = "pci";
> > +            ranges = <0x01000000 0x0 0x00000000 0x2 0x00002000 0x0 0x00010000>,
> > +                     <0x03000000 0x0 0x10000000 0x2 0x10000000 0x1 0xefffffff>;
> > +            num-lanes = <1>;
> > +            interrupts = <GIC_SPI 153 IRQ_TYPE_LEVEL_HIGH>;
> > +            interrupt-names = "msi";
> > +
> > +            sprd,pcie-poweron-syscons =
> > +                <&ap_ipa_ahb_regs 0 0 0x0000 0x40 0x40>,
> > +                <&ap_ipa_ahb_regs 0 0 0x0000 0x20 0x20>;
> > +            sprd,pcie-poweroff-syscons =
>
> Not documented.
>

Thanks! I'll fix it in the next version.

> > +                <&ap_ipa_ahb_regs 0 0 0x0000 0x20 0x0>,
> > +                <&ap_ipa_ahb_regs 0 0 0x0000 0x40 0x0>;
> > +        };
> > +    };
> > --
> > 2.7.4
> >

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2020-09-18  3:18 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-09  9:48 [PATCH v3 0/2] PCI: Add new Unisoc PCIe driver Hongtao Wu
2020-09-09  9:48 ` [PATCH v3 1/2] dt-bindings: PCI: sprd: Document Unisoc PCIe RC host controller Hongtao Wu
2020-09-15 17:25   ` Rob Herring
2020-09-18  3:18     ` Hongtao Wu
2020-09-09  9:48 ` [PATCH v3 2/2] PCI: sprd: Add support for Unisoc SoCs' PCIe controller Hongtao Wu
2020-09-15 17:31   ` Rob Herring
2020-09-18  2:00     ` Hongtao Wu

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).