linux-pci.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 1/2] dt-bindings: rockchip: Add DesignWare based PCIe controller
@ 2021-01-25  2:48 Simon Xue
  2021-01-25  2:49 ` [PATCH v3 2/2] PCI: rockchip: add " Simon Xue
                   ` (3 more replies)
  0 siblings, 4 replies; 15+ messages in thread
From: Simon Xue @ 2021-01-25  2:48 UTC (permalink / raw)
  To: Bjorn Helgaas, Lorenzo Pieralisi
  Cc: linux-pci, linux-rockchip, devicetree, robh+dt, Johan Jonker,
	Heiko Stuebner, Simon Xue

Document DT bindings for PCIe controller found on Rockchip SoC.

Signed-off-by: Simon Xue <xxm@rock-chips.com>
---
 .../bindings/pci/rockchip-dw-pcie.yaml        | 133 ++++++++++++++++++
 1 file changed, 133 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml

diff --git a/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
new file mode 100644
index 000000000000..24ea42203c14
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
@@ -0,0 +1,133 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pci/rockchip-dw-pcie.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: DesignWare based PCIe RC controller on Rockchip SoCs
+
+maintainers:
+  - Shawn Lin <shawn.lin@rock-chips.com>
+  - Simon Xue <xxm@rock-chips.com>
+  - Heiko Stuebner <heiko@sntech.de>
+
+description: |+
+  RK3568 SoC PCIe host controller is based on the Synopsys DesignWare
+  PCIe IP and thus inherits all the common properties defined in
+  designware-pcie.txt.
+
+allOf:
+  - $ref: /schemas/pci/pci-bus.yaml#
+
+# We need a select here so we don't match all nodes with 'snps,dw-pcie'
+select:
+  properties:
+    compatible:
+      contains:
+        const: rockchip,rk3568-pcie
+  required:
+    - compatible
+
+properties:
+  compatible:
+    items:
+      - const: rockchip,rk3568-pcie
+      - const: snps,dw-pcie
+
+  reg:
+    items:
+      - description: Data Bus Interface (DBI) registers
+      - description: Rockchip designed configuration registers
+
+  clocks:
+    items:
+      - description: AHB clock for PCIe master
+      - description: AHB clock for PCIe slave
+      - description: AHB clock for PCIe dbi
+      - description: APB clock for PCIe
+      - description: Auxiliary clock for PCIe
+
+  clock-names:
+    items:
+      - const: aclk_mst
+      - const: aclk_slv
+      - const: aclk_dbi
+      - const: pclk
+      - const: aux
+
+  msi-map: true
+
+  num-lanes: true
+
+  phys:
+    maxItems: 1
+
+  phy-names:
+    const: pcie-phy
+
+  power-domains:
+    maxItems: 1
+
+  ranges:
+    maxItems: 3
+
+  resets:
+    maxItems: 1
+
+  reset-names:
+    const: pipe
+
+required:
+  - compatible
+  - reg
+  - reg-names
+  - clocks
+  - clock-names
+  - msi-map
+  - num-lanes
+  - phys
+  - phy-names
+  - power-domains
+  - resets
+  - reset-names
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+
+    bus {
+        #address-cells = <2>;
+        #size-cells = <2>;
+
+        pcie3x2: pcie@fe280000 {
+            compatible = "rockchip,rk3568-pcie", "snps,dw-pcie";
+            reg = <0x3 0xc0800000 0x0 0x400000>,
+                  <0x0 0xfe280000 0x0 0x10000>;
+            reg-names = "pcie-dbi", "pcie-apb";
+            bus-range = <0x20 0x2f>;
+            clocks = <&cru 143>, <&cru 144>,
+                     <&cru 145>, <&cru 146>,
+                     <&cru 147>;
+            clock-names = "aclk_mst", "aclk_slv",
+                          "aclk_dbi", "pclk",
+                          "aux";
+            device_type = "pci";
+            linux,pci-domain = <2>;
+            max-link-speed = <2>;
+            msi-map = <0x2000 &its 0x2000 0x1000>;
+            num-lanes = <2>;
+            phys = <&pcie30phy>;
+            phy-names = "pcie-phy";
+            power-domains = <&power 15>;
+            ranges = <0x00000800 0x0 0x80000000 0x3 0x80000000 0x0 0x800000>,
+                     <0x81000000 0x0 0x80800000 0x3 0x80800000 0x0 0x100000>,
+                     <0x83000000 0x0 0x80900000 0x3 0x80900000 0x0 0x3f700000>;
+            resets = <&cru 193>;
+            reset-names = "pipe";
+            #address-cells = <3>;
+            #size-cells = <2>;
+        };
+    };
+...
-- 
2.25.1




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

* [PATCH v3 2/2] PCI: rockchip: add DesignWare based PCIe controller
  2021-01-25  2:48 [PATCH v3 1/2] dt-bindings: rockchip: Add DesignWare based PCIe controller Simon Xue
@ 2021-01-25  2:49 ` Simon Xue
  2021-01-25  5:48   ` Leon Romanovsky
  2021-01-25 14:51 ` [PATCH v3 1/2] dt-bindings: rockchip: Add " Rob Herring
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 15+ messages in thread
From: Simon Xue @ 2021-01-25  2:49 UTC (permalink / raw)
  To: Bjorn Helgaas, Lorenzo Pieralisi
  Cc: linux-pci, linux-rockchip, devicetree, robh+dt, Johan Jonker,
	Heiko Stuebner, Simon Xue, Shawn Lin

pcie-dw-rockchip is based on DWC IP. But pcie-rockchip-host
is Rockchip designed IP which is only used for RK3399. So all the following
non-RK3399 SoCs should use this driver.

Signed-off-by: Simon Xue <xxm@rock-chips.com>
Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
---
 drivers/pci/controller/dwc/Kconfig            |   9 +
 drivers/pci/controller/dwc/Makefile           |   1 +
 drivers/pci/controller/dwc/pcie-dw-rockchip.c | 286 ++++++++++++++++++
 3 files changed, 296 insertions(+)
 create mode 100644 drivers/pci/controller/dwc/pcie-dw-rockchip.c

diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
index 22c5529e9a65..aee408fe9283 100644
--- a/drivers/pci/controller/dwc/Kconfig
+++ b/drivers/pci/controller/dwc/Kconfig
@@ -214,6 +214,15 @@ config PCIE_ARTPEC6_EP
 	  Enables support for the PCIe controller in the ARTPEC-6 SoC to work in
 	  endpoint mode. This uses the DesignWare core.
 
+config PCIE_ROCKCHIP_DW_HOST
+	bool "Rockchip DesignWare PCIe controller"
+	select PCIE_DW
+	select PCIE_DW_HOST
+	depends on ARCH_ROCKCHIP || COMPILE_TEST
+	depends on OF
+	help
+	  Enables support for the DW PCIe controller in the Rockchip SoC.
+
 config PCIE_INTEL_GW
 	bool "Intel Gateway PCIe host controller support"
 	depends on OF && (X86 || COMPILE_TEST)
diff --git a/drivers/pci/controller/dwc/Makefile b/drivers/pci/controller/dwc/Makefile
index a751553fa0db..30eef8e9ee8a 100644
--- a/drivers/pci/controller/dwc/Makefile
+++ b/drivers/pci/controller/dwc/Makefile
@@ -13,6 +13,7 @@ obj-$(CONFIG_PCI_LAYERSCAPE_EP) += pci-layerscape-ep.o
 obj-$(CONFIG_PCIE_QCOM) += pcie-qcom.o
 obj-$(CONFIG_PCIE_ARMADA_8K) += pcie-armada8k.o
 obj-$(CONFIG_PCIE_ARTPEC6) += pcie-artpec6.o
+obj-$(CONFIG_PCIE_ROCKCHIP_DW_HOST) += pcie-dw-rockchip.o
 obj-$(CONFIG_PCIE_INTEL_GW) += pcie-intel-gw.o
 obj-$(CONFIG_PCIE_KIRIN) += pcie-kirin.o
 obj-$(CONFIG_PCIE_HISI_STB) += pcie-histb.o
diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
new file mode 100644
index 000000000000..07f6d1cd5853
--- /dev/null
+++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
@@ -0,0 +1,286 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * PCIe host controller driver for Rockchip SoCs
+ *
+ * Copyright (C) 2021 Rockchip Electronics Co., Ltd.
+ *		http://www.rock-chips.com
+ *
+ * Author: Simon Xue <xxm@rock-chips.com>
+ */
+
+#include <linux/clk.h>
+#include <linux/gpio/consumer.h>
+#include <linux/mfd/syscon.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/phy/phy.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/reset.h>
+
+#include "pcie-designware.h"
+
+/*
+ * The upper 16 bits of PCIE_CLIENT_CONFIG are a write
+ * mask for the lower 16 bits.  This allows atomic updates
+ * of the register without locking.
+ */
+#define HIWORD_UPDATE(mask, val) (((mask) << 16) | (val))
+#define HIWORD_UPDATE_BIT(val)	HIWORD_UPDATE(val, val)
+
+#define to_rockchip_pcie(x) dev_get_drvdata((x)->dev)
+
+#define PCIE_CLIENT_RC_MODE		HIWORD_UPDATE_BIT(0x40)
+#define PCIE_CLIENT_ENABLE_LTSSM	HIWORD_UPDATE_BIT(0xc)
+#define PCIE_SMLH_LINKUP		BIT(16)
+#define PCIE_RDLH_LINKUP		BIT(17)
+#define PCIE_L0S_ENTRY			0x11
+#define PCIE_CLIENT_GENERAL_CONTROL	0x0
+#define PCIE_CLIENT_GENERAL_DEBUG	0x104
+#define PCIE_CLIENT_HOT_RESET_CTRL      0x180
+#define PCIE_CLIENT_LTSSM_STATUS	0x300
+#define PCIE_LTSSM_ENABLE_ENHANCE       BIT(4)
+
+struct rockchip_pcie {
+	struct dw_pcie			pci;
+	void __iomem			*apb_base;
+	struct phy			*phy;
+	struct clk_bulk_data		*clks;
+	unsigned int			clk_cnt;
+	struct reset_control		*rst;
+	struct gpio_desc		*rst_gpio;
+	struct regulator                *vpcie3v3;
+};
+
+static int rockchip_pcie_readl_apb(struct rockchip_pcie *rockchip,
+					     u32 reg)
+{
+	return readl(rockchip->apb_base + reg);
+}
+
+static void rockchip_pcie_writel_apb(struct rockchip_pcie *rockchip,
+						u32 val, u32 reg)
+{
+	writel(val, rockchip->apb_base + reg);
+}
+
+static void rockchip_pcie_enable_ltssm(struct rockchip_pcie *rockchip)
+{
+	rockchip_pcie_writel_apb(rockchip, PCIE_CLIENT_ENABLE_LTSSM,
+				 PCIE_CLIENT_GENERAL_CONTROL);
+}
+
+static int rockchip_pcie_link_up(struct dw_pcie *pci)
+{
+	struct rockchip_pcie *rockchip = to_rockchip_pcie(pci);
+	u32 val = rockchip_pcie_readl_apb(rockchip, PCIE_CLIENT_LTSSM_STATUS);
+
+	if ((val & (PCIE_RDLH_LINKUP | PCIE_SMLH_LINKUP)) == 0x30000 &&
+	    (val & GENMASK(5, 0)) == PCIE_L0S_ENTRY)
+		return 1;
+
+	return 0;
+}
+
+static int rockchip_pcie_start_link(struct dw_pcie *pci)
+{
+	struct rockchip_pcie *rockchip = to_rockchip_pcie(pci);
+
+	/* Reset device */
+	gpiod_set_value_cansleep(rockchip->rst_gpio, 0);
+	msleep(100);
+	gpiod_set_value_cansleep(rockchip->rst_gpio, 1);
+
+	rockchip_pcie_enable_ltssm(rockchip);
+
+	return 0;
+}
+
+static void rockchip_pcie_fast_link_setup(struct rockchip_pcie *rockchip)
+{
+	u32 val;
+
+	/* LTSSM EN ctrl mode */
+	val = rockchip_pcie_readl_apb(rockchip, PCIE_CLIENT_HOT_RESET_CTRL);
+	val |= PCIE_LTSSM_ENABLE_ENHANCE | (PCIE_LTSSM_ENABLE_ENHANCE << 16);
+	rockchip_pcie_writel_apb(rockchip, val, PCIE_CLIENT_HOT_RESET_CTRL);
+}
+
+static int rockchip_pcie_host_init(struct pcie_port *pp)
+{
+	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
+	struct rockchip_pcie *rockchip = to_rockchip_pcie(pci);
+
+	rockchip_pcie_fast_link_setup(rockchip);
+
+	rockchip_pcie_writel_apb(rockchip, PCIE_CLIENT_RC_MODE,
+				 PCIE_CLIENT_GENERAL_CONTROL);
+
+	return 0;
+}
+
+static const struct dw_pcie_host_ops rockchip_pcie_host_ops = {
+	.host_init = rockchip_pcie_host_init,
+};
+
+static int rockchip_pcie_clk_init(struct rockchip_pcie *rockchip)
+{
+	struct device *dev = rockchip->pci.dev;
+	int ret;
+
+	ret = devm_clk_bulk_get_all(dev, &rockchip->clks);
+	if (ret < 0)
+		return ret;
+
+	rockchip->clk_cnt = ret;
+
+	ret = clk_bulk_prepare_enable(rockchip->clk_cnt, rockchip->clks);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int rockchip_pcie_resource_get(struct platform_device *pdev,
+				      struct rockchip_pcie *rockchip)
+{
+	rockchip->apb_base = devm_platform_ioremap_resource_byname(pdev, "apb");
+	if (IS_ERR(rockchip->apb_base))
+		return PTR_ERR(rockchip->apb_base);
+
+	rockchip->rst_gpio = devm_gpiod_get_optional(&pdev->dev, "reset",
+						     GPIOD_OUT_HIGH);
+	if (IS_ERR(rockchip->rst_gpio))
+		return PTR_ERR(rockchip->rst_gpio);
+
+	return 0;
+}
+
+static int rockchip_pcie_phy_init(struct rockchip_pcie *rockchip)
+{
+	int ret;
+	struct device *dev = rockchip->pci.dev;
+
+	rockchip->phy = devm_phy_get(dev, "pcie-phy");
+	if (IS_ERR(rockchip->phy))
+		return dev_err_probe(dev, PTR_ERR(rockchip->phy),
+				     "missing phy\n");
+
+	ret = phy_init(rockchip->phy);
+	if (ret < 0)
+		return ret;
+
+	phy_power_on(rockchip->phy);
+
+	return 0;
+}
+
+static void rockchip_pcie_phy_deinit(struct rockchip_pcie *rockchip)
+{
+	phy_exit(rockchip->phy);
+	phy_power_off(rockchip->phy);
+}
+
+static int rockchip_pcie_reset_control_release(struct rockchip_pcie *rockchip)
+{
+	struct device *dev = rockchip->pci.dev;
+	int ret;
+
+	rockchip->rst = devm_reset_control_array_get_exclusive(dev);
+	if (IS_ERR(rockchip->rst))
+		return dev_err_probe(dev, PTR_ERR(rockchip->rst),
+				     "failed to get reset lines\n");
+
+	ret = reset_control_deassert(rockchip->rst);
+
+	return ret;
+}
+
+static const struct dw_pcie_ops dw_pcie_ops = {
+	.link_up = rockchip_pcie_link_up,
+	.start_link = rockchip_pcie_start_link,
+};
+
+static int rockchip_pcie_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct rockchip_pcie *rockchip;
+	struct pcie_port *pp;
+	int ret;
+
+	rockchip = devm_kzalloc(dev, sizeof(*rockchip), GFP_KERNEL);
+	if (!rockchip)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, rockchip);
+
+	rockchip->pci.dev = dev;
+	rockchip->pci.ops = &dw_pcie_ops;
+
+	pp = &rockchip->pci.pp;
+	pp->ops = &rockchip_pcie_host_ops;
+
+	ret = rockchip_pcie_resource_get(pdev, rockchip);
+	if (ret)
+		return ret;
+
+	/* DON'T MOVE ME: must be enable before phy init */
+	rockchip->vpcie3v3 = devm_regulator_get_optional(dev, "vpcie3v3");
+	if (IS_ERR(rockchip->vpcie3v3))
+		return dev_err_probe(dev, PTR_ERR(rockchip->rst),
+				     "failed to get vpcie3v3 regulator\n");
+
+	if (rockchip->vpcie3v3) {
+		ret = regulator_enable(rockchip->vpcie3v3);
+		if (ret) {
+			dev_err(dev, "fail to enable vpcie3v3 regulator\n");
+			return ret;
+		}
+	}
+
+	ret = rockchip_pcie_phy_init(rockchip);
+	if (ret)
+		goto disable_regulator;
+
+	ret = rockchip_pcie_reset_control_release(rockchip);
+	if (ret)
+		goto deinit_phy;
+
+	ret = rockchip_pcie_clk_init(rockchip);
+	if (ret)
+		goto deinit_phy;
+
+	ret = dw_pcie_host_init(pp);
+	if (ret)
+		goto deinit_clk;
+
+	return 0;
+
+deinit_clk:
+	clk_bulk_disable_unprepare(rockchip->clk_cnt, rockchip->clks);
+deinit_phy:
+	rockchip_pcie_phy_deinit(rockchip);
+disable_regulator:
+	if (rockchip->vpcie3v3)
+		regulator_disable(rockchip->vpcie3v3);
+
+	return ret;
+}
+
+MODULE_DEVICE_TABLE(of, rockchip_pcie_of_match);
+
+static const struct of_device_id rockchip_pcie_of_match[] = {
+	{ .compatible = "rockchip,rk3568-pcie", },
+	{ /* sentinel */ },
+};
+
+static struct platform_driver rockchip_pcie_driver = {
+	.driver = {
+		.name	= "rockchip-dw-pcie",
+		.of_match_table = rockchip_pcie_of_match,
+		.suppress_bind_attrs = true,
+	},
+	.probe = rockchip_pcie_probe,
+};
+
+builtin_platform_driver(rockchip_pcie_driver);
-- 
2.25.1




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

* Re: [PATCH v3 2/2] PCI: rockchip: add DesignWare based PCIe controller
  2021-01-25  2:49 ` [PATCH v3 2/2] PCI: rockchip: add " Simon Xue
@ 2021-01-25  5:48   ` Leon Romanovsky
  2021-01-25  6:40     ` xxm
  0 siblings, 1 reply; 15+ messages in thread
From: Leon Romanovsky @ 2021-01-25  5:48 UTC (permalink / raw)
  To: Simon Xue
  Cc: Bjorn Helgaas, Lorenzo Pieralisi, linux-pci, linux-rockchip,
	devicetree, robh+dt, Johan Jonker, Heiko Stuebner, Shawn Lin

On Mon, Jan 25, 2021 at 10:49:27AM +0800, Simon Xue wrote:
> pcie-dw-rockchip is based on DWC IP. But pcie-rockchip-host
> is Rockchip designed IP which is only used for RK3399. So all the following
> non-RK3399 SoCs should use this driver.
>
> Signed-off-by: Simon Xue <xxm@rock-chips.com>
> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
> ---
>  drivers/pci/controller/dwc/Kconfig            |   9 +
>  drivers/pci/controller/dwc/Makefile           |   1 +
>  drivers/pci/controller/dwc/pcie-dw-rockchip.c | 286 ++++++++++++++++++
>  3 files changed, 296 insertions(+)
>  create mode 100644 drivers/pci/controller/dwc/pcie-dw-rockchip.c
>
> diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
> index 22c5529e9a65..aee408fe9283 100644
> --- a/drivers/pci/controller/dwc/Kconfig
> +++ b/drivers/pci/controller/dwc/Kconfig
> @@ -214,6 +214,15 @@ config PCIE_ARTPEC6_EP
>  	  Enables support for the PCIe controller in the ARTPEC-6 SoC to work in
>  	  endpoint mode. This uses the DesignWare core.
>
> +config PCIE_ROCKCHIP_DW_HOST
> +	bool "Rockchip DesignWare PCIe controller"
> +	select PCIE_DW
> +	select PCIE_DW_HOST
> +	depends on ARCH_ROCKCHIP || COMPILE_TEST
> +	depends on OF
> +	help
> +	  Enables support for the DW PCIe controller in the Rockchip SoC.
> +
>  config PCIE_INTEL_GW
>  	bool "Intel Gateway PCIe host controller support"
>  	depends on OF && (X86 || COMPILE_TEST)
> diff --git a/drivers/pci/controller/dwc/Makefile b/drivers/pci/controller/dwc/Makefile
> index a751553fa0db..30eef8e9ee8a 100644
> --- a/drivers/pci/controller/dwc/Makefile
> +++ b/drivers/pci/controller/dwc/Makefile
> @@ -13,6 +13,7 @@ obj-$(CONFIG_PCI_LAYERSCAPE_EP) += pci-layerscape-ep.o
>  obj-$(CONFIG_PCIE_QCOM) += pcie-qcom.o
>  obj-$(CONFIG_PCIE_ARMADA_8K) += pcie-armada8k.o
>  obj-$(CONFIG_PCIE_ARTPEC6) += pcie-artpec6.o
> +obj-$(CONFIG_PCIE_ROCKCHIP_DW_HOST) += pcie-dw-rockchip.o
>  obj-$(CONFIG_PCIE_INTEL_GW) += pcie-intel-gw.o
>  obj-$(CONFIG_PCIE_KIRIN) += pcie-kirin.o
>  obj-$(CONFIG_PCIE_HISI_STB) += pcie-histb.o
> diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> new file mode 100644
> index 000000000000..07f6d1cd5853
> --- /dev/null
> +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> @@ -0,0 +1,286 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * PCIe host controller driver for Rockchip SoCs
> + *
> + * Copyright (C) 2021 Rockchip Electronics Co., Ltd.
> + *		http://www.rock-chips.com
> + *
> + * Author: Simon Xue <xxm@rock-chips.com>
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/phy/phy.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/reset.h>
> +
> +#include "pcie-designware.h"
> +
> +/*
> + * The upper 16 bits of PCIE_CLIENT_CONFIG are a write
> + * mask for the lower 16 bits.  This allows atomic updates
> + * of the register without locking.
> + */

This is correct only for the variables that naturally aligned, I imagine
that this is the case here and in the Linux, but better do not write comments
in the code that are not accurate.

Thanks

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

* Re: [PATCH v3 2/2] PCI: rockchip: add DesignWare based PCIe controller
  2021-01-25  5:48   ` Leon Romanovsky
@ 2021-01-25  6:40     ` xxm
  2021-01-25  9:01       ` Leon Romanovsky
  0 siblings, 1 reply; 15+ messages in thread
From: xxm @ 2021-01-25  6:40 UTC (permalink / raw)
  To: Leon Romanovsky
  Cc: Bjorn Helgaas, Lorenzo Pieralisi, linux-pci, linux-rockchip,
	devicetree, robh+dt, Johan Jonker, Heiko Stuebner, Shawn Lin

Hi Leon,

Thanks for your reply.

在 2021/1/25 13:48, Leon Romanovsky 写道:
> On Mon, Jan 25, 2021 at 10:49:27AM +0800, Simon Xue wrote:
>> pcie-dw-rockchip is based on DWC IP. But pcie-rockchip-host
>> is Rockchip designed IP which is only used for RK3399. So all the following
>> non-RK3399 SoCs should use this driver.
>>
>> Signed-off-by: Simon Xue <xxm@rock-chips.com>
>> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
>> ---
>>   drivers/pci/controller/dwc/Kconfig            |   9 +
>>   drivers/pci/controller/dwc/Makefile           |   1 +
>>   drivers/pci/controller/dwc/pcie-dw-rockchip.c | 286 ++++++++++++++++++
>>   3 files changed, 296 insertions(+)
>>   create mode 100644 drivers/pci/controller/dwc/pcie-dw-rockchip.c
>>
>> diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
>> index 22c5529e9a65..aee408fe9283 100644
>> --- a/drivers/pci/controller/dwc/Kconfig
>> +++ b/drivers/pci/controller/dwc/Kconfig
>> @@ -214,6 +214,15 @@ config PCIE_ARTPEC6_EP
>>   	  Enables support for the PCIe controller in the ARTPEC-6 SoC to work in
>>   	  endpoint mode. This uses the DesignWare core.
>>
>> +config PCIE_ROCKCHIP_DW_HOST
>> +	bool "Rockchip DesignWare PCIe controller"
>> +	select PCIE_DW
>> +	select PCIE_DW_HOST
>> +	depends on ARCH_ROCKCHIP || COMPILE_TEST
>> +	depends on OF
>> +	help
>> +	  Enables support for the DW PCIe controller in the Rockchip SoC.
>> +
>>   config PCIE_INTEL_GW
>>   	bool "Intel Gateway PCIe host controller support"
>>   	depends on OF && (X86 || COMPILE_TEST)
>> diff --git a/drivers/pci/controller/dwc/Makefile b/drivers/pci/controller/dwc/Makefile
>> index a751553fa0db..30eef8e9ee8a 100644
>> --- a/drivers/pci/controller/dwc/Makefile
>> +++ b/drivers/pci/controller/dwc/Makefile
>> @@ -13,6 +13,7 @@ obj-$(CONFIG_PCI_LAYERSCAPE_EP) += pci-layerscape-ep.o
>>   obj-$(CONFIG_PCIE_QCOM) += pcie-qcom.o
>>   obj-$(CONFIG_PCIE_ARMADA_8K) += pcie-armada8k.o
>>   obj-$(CONFIG_PCIE_ARTPEC6) += pcie-artpec6.o
>> +obj-$(CONFIG_PCIE_ROCKCHIP_DW_HOST) += pcie-dw-rockchip.o
>>   obj-$(CONFIG_PCIE_INTEL_GW) += pcie-intel-gw.o
>>   obj-$(CONFIG_PCIE_KIRIN) += pcie-kirin.o
>>   obj-$(CONFIG_PCIE_HISI_STB) += pcie-histb.o
>> diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
>> new file mode 100644
>> index 000000000000..07f6d1cd5853
>> --- /dev/null
>> +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
>> @@ -0,0 +1,286 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * PCIe host controller driver for Rockchip SoCs
>> + *
>> + * Copyright (C) 2021 Rockchip Electronics Co., Ltd.
>> + *		http://www.rock-chips.com
>> + *
>> + * Author: Simon Xue <xxm@rock-chips.com>
>> + */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/gpio/consumer.h>
>> +#include <linux/mfd/syscon.h>
>> +#include <linux/module.h>
>> +#include <linux/of_device.h>
>> +#include <linux/phy/phy.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/regmap.h>
>> +#include <linux/reset.h>
>> +
>> +#include "pcie-designware.h"
>> +
>> +/*
>> + * The upper 16 bits of PCIE_CLIENT_CONFIG are a write
>> + * mask for the lower 16 bits.  This allows atomic updates
>> + * of the register without locking.
>> + */
> This is correct only for the variables that naturally aligned, I imagine
> that this is the case here and in the Linux, but better do not write comments
> in the code that are not accurate.

Ok, will remove.
I wonder what it would be when outside the Linux? Could you share some information?

> Thanks
>
>
>



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

* Re: [PATCH v3 2/2] PCI: rockchip: add DesignWare based PCIe controller
  2021-01-25  6:40     ` xxm
@ 2021-01-25  9:01       ` Leon Romanovsky
  2021-01-25 15:53         ` Robin Murphy
  2021-01-26 14:52         ` Rob Herring
  0 siblings, 2 replies; 15+ messages in thread
From: Leon Romanovsky @ 2021-01-25  9:01 UTC (permalink / raw)
  To: xxm
  Cc: Bjorn Helgaas, Lorenzo Pieralisi, linux-pci, linux-rockchip,
	devicetree, robh+dt, Johan Jonker, Heiko Stuebner, Shawn Lin

On Mon, Jan 25, 2021 at 02:40:10PM +0800, xxm wrote:
> Hi Leon,
>
> Thanks for your reply.
>
> 在 2021/1/25 13:48, Leon Romanovsky 写道:
> > On Mon, Jan 25, 2021 at 10:49:27AM +0800, Simon Xue wrote:
> > > pcie-dw-rockchip is based on DWC IP. But pcie-rockchip-host
> > > is Rockchip designed IP which is only used for RK3399. So all the following
> > > non-RK3399 SoCs should use this driver.
> > >
> > > Signed-off-by: Simon Xue <xxm@rock-chips.com>
> > > Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
> > > ---
> > >   drivers/pci/controller/dwc/Kconfig            |   9 +
> > >   drivers/pci/controller/dwc/Makefile           |   1 +
> > >   drivers/pci/controller/dwc/pcie-dw-rockchip.c | 286 ++++++++++++++++++
> > >   3 files changed, 296 insertions(+)
> > >   create mode 100644 drivers/pci/controller/dwc/pcie-dw-rockchip.c
> > >
> > > diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
> > > index 22c5529e9a65..aee408fe9283 100644
> > > --- a/drivers/pci/controller/dwc/Kconfig
> > > +++ b/drivers/pci/controller/dwc/Kconfig
> > > @@ -214,6 +214,15 @@ config PCIE_ARTPEC6_EP
> > >   	  Enables support for the PCIe controller in the ARTPEC-6 SoC to work in
> > >   	  endpoint mode. This uses the DesignWare core.
> > >
> > > +config PCIE_ROCKCHIP_DW_HOST
> > > +	bool "Rockchip DesignWare PCIe controller"
> > > +	select PCIE_DW
> > > +	select PCIE_DW_HOST
> > > +	depends on ARCH_ROCKCHIP || COMPILE_TEST
> > > +	depends on OF
> > > +	help
> > > +	  Enables support for the DW PCIe controller in the Rockchip SoC.
> > > +
> > >   config PCIE_INTEL_GW
> > >   	bool "Intel Gateway PCIe host controller support"
> > >   	depends on OF && (X86 || COMPILE_TEST)
> > > diff --git a/drivers/pci/controller/dwc/Makefile b/drivers/pci/controller/dwc/Makefile
> > > index a751553fa0db..30eef8e9ee8a 100644
> > > --- a/drivers/pci/controller/dwc/Makefile
> > > +++ b/drivers/pci/controller/dwc/Makefile
> > > @@ -13,6 +13,7 @@ obj-$(CONFIG_PCI_LAYERSCAPE_EP) += pci-layerscape-ep.o
> > >   obj-$(CONFIG_PCIE_QCOM) += pcie-qcom.o
> > >   obj-$(CONFIG_PCIE_ARMADA_8K) += pcie-armada8k.o
> > >   obj-$(CONFIG_PCIE_ARTPEC6) += pcie-artpec6.o
> > > +obj-$(CONFIG_PCIE_ROCKCHIP_DW_HOST) += pcie-dw-rockchip.o
> > >   obj-$(CONFIG_PCIE_INTEL_GW) += pcie-intel-gw.o
> > >   obj-$(CONFIG_PCIE_KIRIN) += pcie-kirin.o
> > >   obj-$(CONFIG_PCIE_HISI_STB) += pcie-histb.o
> > > diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> > > new file mode 100644
> > > index 000000000000..07f6d1cd5853
> > > --- /dev/null
> > > +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> > > @@ -0,0 +1,286 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * PCIe host controller driver for Rockchip SoCs
> > > + *
> > > + * Copyright (C) 2021 Rockchip Electronics Co., Ltd.
> > > + *		http://www.rock-chips.com
> > > + *
> > > + * Author: Simon Xue <xxm@rock-chips.com>
> > > + */
> > > +
> > > +#include <linux/clk.h>
> > > +#include <linux/gpio/consumer.h>
> > > +#include <linux/mfd/syscon.h>
> > > +#include <linux/module.h>
> > > +#include <linux/of_device.h>
> > > +#include <linux/phy/phy.h>
> > > +#include <linux/platform_device.h>
> > > +#include <linux/regmap.h>
> > > +#include <linux/reset.h>
> > > +
> > > +#include "pcie-designware.h"
> > > +
> > > +/*
> > > + * The upper 16 bits of PCIE_CLIENT_CONFIG are a write
> > > + * mask for the lower 16 bits.  This allows atomic updates
> > > + * of the register without locking.
> > > + */
> > This is correct only for the variables that naturally aligned, I imagine
> > that this is the case here and in the Linux, but better do not write comments
> > in the code that are not accurate.
>
> Ok, will remove.
> I wonder what it would be when outside the Linux? Could you share some information?

The C standard says nothing about atomicity, integer assignment maybe atomic,
maybe it isn’t. There is no guarantee, plain integer assignment in C is non-atomic
by definition.

The atomicity of u32 is very dependent on hardware vendor, memory model and compiler,
for example x86 and ARMs guarantee atomicity for u32. This is why I said that probably
here (Linux) it is ok and you are not alone in expecting lockless write.

Thanks

>
> > Thanks
> >
> >
> >
>
>

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

* Re: [PATCH v3 1/2] dt-bindings: rockchip: Add DesignWare based PCIe controller
  2021-01-25  2:48 [PATCH v3 1/2] dt-bindings: rockchip: Add DesignWare based PCIe controller Simon Xue
  2021-01-25  2:49 ` [PATCH v3 2/2] PCI: rockchip: add " Simon Xue
@ 2021-01-25 14:51 ` Rob Herring
  2021-01-25 15:26 ` Rob Herring
  2021-01-25 15:55 ` [PATCH v3 1/2] dt-bindings: rockchip: Add DesignWare based PCIe controller Johan Jonker
  3 siblings, 0 replies; 15+ messages in thread
From: Rob Herring @ 2021-01-25 14:51 UTC (permalink / raw)
  To: Simon Xue
  Cc: Bjorn Helgaas, robh+dt, Heiko Stuebner, linux-pci, Johan Jonker,
	devicetree, linux-rockchip, Lorenzo Pieralisi

On Mon, 25 Jan 2021 10:48:24 +0800, Simon Xue wrote:
> Document DT bindings for PCIe controller found on Rockchip SoC.
> 
> Signed-off-by: Simon Xue <xxm@rock-chips.com>
> ---
>  .../bindings/pci/rockchip-dw-pcie.yaml        | 133 ++++++++++++++++++
>  1 file changed, 133 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.example.dt.yaml: pcie@fe280000: ranges: 'oneOf' conditional failed, one must be fixed:
	/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.example.dt.yaml: pcie@fe280000: ranges: 'oneOf' conditional failed, one must be fixed:
		[[2048, 0, 2147483648, 3, 2147483648, 0, 8388608], [2164260864, 0, 2155872256, 3, 2155872256, 0, 1048576], [2197815296, 0, 2156920832, 3, 2156920832, 0, 1064304640]] is not of type 'boolean'
		True was expected
		[[2048, 0, 2147483648, 3, 2147483648, 0, 8388608], [2164260864, 0, 2155872256, 3, 2155872256, 0, 1048576], [2197815296, 0, 2156920832, 3, 2156920832, 0, 1064304640]] is not of type 'null'
	2048 is not one of [16777216, 33554432, 50331648, 1107296256, 1124073472, 2164260864, 2181038080, 2197815296, 3254779904, 3271557120]
	From schema: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.example.dt.yaml: pcie@fe280000: ranges: 'oneOf' conditional failed, one must be fixed:
	/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.example.dt.yaml: pcie@fe280000: ranges: 'oneOf' conditional failed, one must be fixed:
		[[2048, 0, 2147483648, 3, 2147483648, 0, 8388608], [2164260864, 0, 2155872256, 3, 2155872256, 0, 1048576], [2197815296, 0, 2156920832, 3, 2156920832, 0, 1064304640]] is not of type 'boolean'
		True was expected
		[[2048, 0, 2147483648, 3, 2147483648, 0, 8388608], [2164260864, 0, 2155872256, 3, 2155872256, 0, 1048576], [2197815296, 0, 2156920832, 3, 2156920832, 0, 1064304640]] is not of type 'null'
	2048 is not one of [16777216, 33554432, 50331648, 1107296256, 1124073472, 2164260864, 2181038080, 2197815296, 3254779904, 3271557120]
	From schema: /usr/local/lib/python3.8/dist-packages/dtschema/schemas/pci/pci-bus.yaml

See https://patchwork.ozlabs.org/patch/1431082

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.


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

* Re: [PATCH v3 1/2] dt-bindings: rockchip: Add DesignWare based PCIe controller
  2021-01-25  2:48 [PATCH v3 1/2] dt-bindings: rockchip: Add DesignWare based PCIe controller Simon Xue
  2021-01-25  2:49 ` [PATCH v3 2/2] PCI: rockchip: add " Simon Xue
  2021-01-25 14:51 ` [PATCH v3 1/2] dt-bindings: rockchip: Add " Rob Herring
@ 2021-01-25 15:26 ` Rob Herring
  2021-01-26  2:44   ` [PATCH v3 1/2] dt-bindings: rockchip: Add DesignWare based PCIe controller【请注意,邮件由robherring2@gmail.com代发】 xxm
  2021-01-25 15:55 ` [PATCH v3 1/2] dt-bindings: rockchip: Add DesignWare based PCIe controller Johan Jonker
  3 siblings, 1 reply; 15+ messages in thread
From: Rob Herring @ 2021-01-25 15:26 UTC (permalink / raw)
  To: Simon Xue
  Cc: Bjorn Helgaas, Lorenzo Pieralisi, linux-pci, linux-rockchip,
	devicetree, Johan Jonker, Heiko Stuebner

On Mon, Jan 25, 2021 at 10:48:24AM +0800, Simon Xue wrote:
> Document DT bindings for PCIe controller found on Rockchip SoC.
> 
> Signed-off-by: Simon Xue <xxm@rock-chips.com>
> ---
>  .../bindings/pci/rockchip-dw-pcie.yaml        | 133 ++++++++++++++++++
>  1 file changed, 133 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
> 
> diff --git a/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
> new file mode 100644
> index 000000000000..24ea42203c14
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
> @@ -0,0 +1,133 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/pci/rockchip-dw-pcie.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: DesignWare based PCIe RC controller on Rockchip SoCs
> +
> +maintainers:
> +  - Shawn Lin <shawn.lin@rock-chips.com>
> +  - Simon Xue <xxm@rock-chips.com>
> +  - Heiko Stuebner <heiko@sntech.de>
> +
> +description: |+
> +  RK3568 SoC PCIe host controller is based on the Synopsys DesignWare
> +  PCIe IP and thus inherits all the common properties defined in
> +  designware-pcie.txt.
> +
> +allOf:
> +  - $ref: /schemas/pci/pci-bus.yaml#
> +
> +# We need a select here so we don't match all nodes with 'snps,dw-pcie'
> +select:
> +  properties:
> +    compatible:
> +      contains:
> +        const: rockchip,rk3568-pcie
> +  required:
> +    - compatible
> +
> +properties:
> +  compatible:
> +    items:
> +      - const: rockchip,rk3568-pcie
> +      - const: snps,dw-pcie
> +
> +  reg:
> +    items:
> +      - description: Data Bus Interface (DBI) registers
> +      - description: Rockchip designed configuration registers
> +
> +  clocks:
> +    items:
> +      - description: AHB clock for PCIe master
> +      - description: AHB clock for PCIe slave
> +      - description: AHB clock for PCIe dbi
> +      - description: APB clock for PCIe
> +      - description: Auxiliary clock for PCIe
> +
> +  clock-names:
> +    items:
> +      - const: aclk_mst
> +      - const: aclk_slv
> +      - const: aclk_dbi
> +      - const: pclk
> +      - const: aux
> +
> +  msi-map: true
> +
> +  num-lanes: true
> +
> +  phys:
> +    maxItems: 1
> +
> +  phy-names:
> +    const: pcie-phy
> +
> +  power-domains:
> +    maxItems: 1
> +
> +  ranges:
> +    maxItems: 3
> +
> +  resets:
> +    maxItems: 1
> +
> +  reset-names:
> +    const: pipe
> +
> +required:
> +  - compatible
> +  - reg
> +  - reg-names
> +  - clocks
> +  - clock-names
> +  - msi-map
> +  - num-lanes
> +  - phys
> +  - phy-names
> +  - power-domains
> +  - resets
> +  - reset-names
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +
> +    bus {
> +        #address-cells = <2>;
> +        #size-cells = <2>;
> +
> +        pcie3x2: pcie@fe280000 {
> +            compatible = "rockchip,rk3568-pcie", "snps,dw-pcie";
> +            reg = <0x3 0xc0800000 0x0 0x400000>,
> +                  <0x0 0xfe280000 0x0 0x10000>;
> +            reg-names = "pcie-dbi", "pcie-apb";

I believe I already said use 'dbi'. The DBI is also not 4MB. The config 
space goes here too, not in 'ranges'.

> +            bus-range = <0x20 0x2f>;
> +            clocks = <&cru 143>, <&cru 144>,
> +                     <&cru 145>, <&cru 146>,
> +                     <&cru 147>;
> +            clock-names = "aclk_mst", "aclk_slv",
> +                          "aclk_dbi", "pclk",
> +                          "aux";
> +            device_type = "pci";
> +            linux,pci-domain = <2>;
> +            max-link-speed = <2>;
> +            msi-map = <0x2000 &its 0x2000 0x1000>;
> +            num-lanes = <2>;
> +            phys = <&pcie30phy>;
> +            phy-names = "pcie-phy";
> +            power-domains = <&power 15>;
> +            ranges = <0x00000800 0x0 0x80000000 0x3 0x80000000 0x0 0x800000>,
> +                     <0x81000000 0x0 0x80800000 0x3 0x80800000 0x0 0x100000>,
> +                     <0x83000000 0x0 0x80900000 0x3 0x80900000 0x0 0x3f700000>;
> +            resets = <&cru 193>;
> +            reset-names = "pipe";
> +            #address-cells = <3>;
> +            #size-cells = <2>;
> +        };
> +    };
> +...
> -- 
> 2.25.1
> 
> 
> 

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

* Re: [PATCH v3 2/2] PCI: rockchip: add DesignWare based PCIe controller
  2021-01-25  9:01       ` Leon Romanovsky
@ 2021-01-25 15:53         ` Robin Murphy
  2021-01-25 18:45           ` Leon Romanovsky
  2021-01-26 14:52         ` Rob Herring
  1 sibling, 1 reply; 15+ messages in thread
From: Robin Murphy @ 2021-01-25 15:53 UTC (permalink / raw)
  To: Leon Romanovsky, xxm
  Cc: devicetree, Lorenzo Pieralisi, Heiko Stuebner, linux-pci,
	Shawn Lin, linux-rockchip, robh+dt, Bjorn Helgaas, Johan Jonker

On 2021-01-25 09:01, Leon Romanovsky wrote:
> On Mon, Jan 25, 2021 at 02:40:10PM +0800, xxm wrote:
>> Hi Leon,
>>
>> Thanks for your reply.
>>
>> 在 2021/1/25 13:48, Leon Romanovsky 写道:
>>> On Mon, Jan 25, 2021 at 10:49:27AM +0800, Simon Xue wrote:
>>>> pcie-dw-rockchip is based on DWC IP. But pcie-rockchip-host
>>>> is Rockchip designed IP which is only used for RK3399. So all the following
>>>> non-RK3399 SoCs should use this driver.
>>>>
>>>> Signed-off-by: Simon Xue <xxm@rock-chips.com>
>>>> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
>>>> ---
>>>>    drivers/pci/controller/dwc/Kconfig            |   9 +
>>>>    drivers/pci/controller/dwc/Makefile           |   1 +
>>>>    drivers/pci/controller/dwc/pcie-dw-rockchip.c | 286 ++++++++++++++++++
>>>>    3 files changed, 296 insertions(+)
>>>>    create mode 100644 drivers/pci/controller/dwc/pcie-dw-rockchip.c
>>>>
>>>> diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
>>>> index 22c5529e9a65..aee408fe9283 100644
>>>> --- a/drivers/pci/controller/dwc/Kconfig
>>>> +++ b/drivers/pci/controller/dwc/Kconfig
>>>> @@ -214,6 +214,15 @@ config PCIE_ARTPEC6_EP
>>>>    	  Enables support for the PCIe controller in the ARTPEC-6 SoC to work in
>>>>    	  endpoint mode. This uses the DesignWare core.
>>>>
>>>> +config PCIE_ROCKCHIP_DW_HOST
>>>> +	bool "Rockchip DesignWare PCIe controller"
>>>> +	select PCIE_DW
>>>> +	select PCIE_DW_HOST
>>>> +	depends on ARCH_ROCKCHIP || COMPILE_TEST
>>>> +	depends on OF
>>>> +	help
>>>> +	  Enables support for the DW PCIe controller in the Rockchip SoC.
>>>> +
>>>>    config PCIE_INTEL_GW
>>>>    	bool "Intel Gateway PCIe host controller support"
>>>>    	depends on OF && (X86 || COMPILE_TEST)
>>>> diff --git a/drivers/pci/controller/dwc/Makefile b/drivers/pci/controller/dwc/Makefile
>>>> index a751553fa0db..30eef8e9ee8a 100644
>>>> --- a/drivers/pci/controller/dwc/Makefile
>>>> +++ b/drivers/pci/controller/dwc/Makefile
>>>> @@ -13,6 +13,7 @@ obj-$(CONFIG_PCI_LAYERSCAPE_EP) += pci-layerscape-ep.o
>>>>    obj-$(CONFIG_PCIE_QCOM) += pcie-qcom.o
>>>>    obj-$(CONFIG_PCIE_ARMADA_8K) += pcie-armada8k.o
>>>>    obj-$(CONFIG_PCIE_ARTPEC6) += pcie-artpec6.o
>>>> +obj-$(CONFIG_PCIE_ROCKCHIP_DW_HOST) += pcie-dw-rockchip.o
>>>>    obj-$(CONFIG_PCIE_INTEL_GW) += pcie-intel-gw.o
>>>>    obj-$(CONFIG_PCIE_KIRIN) += pcie-kirin.o
>>>>    obj-$(CONFIG_PCIE_HISI_STB) += pcie-histb.o
>>>> diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
>>>> new file mode 100644
>>>> index 000000000000..07f6d1cd5853
>>>> --- /dev/null
>>>> +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
>>>> @@ -0,0 +1,286 @@
>>>> +// SPDX-License-Identifier: GPL-2.0
>>>> +/*
>>>> + * PCIe host controller driver for Rockchip SoCs
>>>> + *
>>>> + * Copyright (C) 2021 Rockchip Electronics Co., Ltd.
>>>> + *		http://www.rock-chips.com
>>>> + *
>>>> + * Author: Simon Xue <xxm@rock-chips.com>
>>>> + */
>>>> +
>>>> +#include <linux/clk.h>
>>>> +#include <linux/gpio/consumer.h>
>>>> +#include <linux/mfd/syscon.h>
>>>> +#include <linux/module.h>
>>>> +#include <linux/of_device.h>
>>>> +#include <linux/phy/phy.h>
>>>> +#include <linux/platform_device.h>
>>>> +#include <linux/regmap.h>
>>>> +#include <linux/reset.h>
>>>> +
>>>> +#include "pcie-designware.h"
>>>> +
>>>> +/*
>>>> + * The upper 16 bits of PCIE_CLIENT_CONFIG are a write
>>>> + * mask for the lower 16 bits.  This allows atomic updates
>>>> + * of the register without locking.
>>>> + */
>>> This is correct only for the variables that naturally aligned, I imagine
>>> that this is the case here and in the Linux, but better do not write comments
>>> in the code that are not accurate.
>>
>> Ok, will remove.
>> I wonder what it would be when outside the Linux? Could you share some information?
> 
> The C standard says nothing about atomicity, integer assignment maybe atomic,
> maybe it isn’t. There is no guarantee, plain integer assignment in C is non-atomic
> by definition.
> 
> The atomicity of u32 is very dependent on hardware vendor, memory model and compiler,
> for example x86 and ARMs guarantee atomicity for u32. This is why I said that probably
> here (Linux) it is ok and you are not alone in expecting lockless write.

Huh? What do variables and the abstract machine of the C language 
environment have to do with the definition of *hardware MMIO registers*? 
We don't write to registers with plain integer assignment of u32, we use 
writel() (precisely in order to bypass that abstract C environment).

I appreciate that the comment is not universally true if taken 
completely out of context, but I that's true of pretty much all comments 
ever. If someone really were trying to learn basic programming 
principles from random comments in Linux drivers, then it's already a 
bit late for us to try and save them from themselves.

32-bit writes to these registers *will* be aligned, because the hardware 
decodes them at 32-bit-aligned addresses and there is nothing that can 
change that other than deliberately modifying the RTL in order to waste 
a large amount money fabbing a special broken version of the SoC. It can 
also be safely assumed that 32-bit writes to whichever part of the SoC 
memory map this device is placed *will* be issued atomically by the CPU 
and propagated atomically by the interconnect, because any SoCs 
integrating this device (or pretty much any modern peripheral IP) must 
be designed to meet those requirements for it to work correctly at all.

Robin.

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

* Re: [PATCH v3 1/2] dt-bindings: rockchip: Add DesignWare based PCIe controller
  2021-01-25  2:48 [PATCH v3 1/2] dt-bindings: rockchip: Add DesignWare based PCIe controller Simon Xue
                   ` (2 preceding siblings ...)
  2021-01-25 15:26 ` Rob Herring
@ 2021-01-25 15:55 ` Johan Jonker
  3 siblings, 0 replies; 15+ messages in thread
From: Johan Jonker @ 2021-01-25 15:55 UTC (permalink / raw)
  To: Simon Xue, Bjorn Helgaas, Lorenzo Pieralisi
  Cc: linux-pci, linux-rockchip, devicetree, robh+dt, Heiko Stuebner

Thanks you for version 3.

A few comments, have a look if it is useful or that you disagree.

On 1/25/21 3:48 AM, Simon Xue wrote:
> Document DT bindings for PCIe controller found on Rockchip SoC.
> 
> Signed-off-by: Simon Xue <xxm@rock-chips.com>
> ---
>  .../bindings/pci/rockchip-dw-pcie.yaml        | 133 ++++++++++++++++++
>  1 file changed, 133 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
> 
> diff --git a/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
> new file mode 100644
> index 000000000000..24ea42203c14
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
> @@ -0,0 +1,133 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/pci/rockchip-dw-pcie.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: DesignWare based PCIe RC controller on Rockchip SoCs
> +
> +maintainers:
> +  - Shawn Lin <shawn.lin@rock-chips.com>
> +  - Simon Xue <xxm@rock-chips.com>
> +  - Heiko Stuebner <heiko@sntech.de>
> +
> +description: |+
> +  RK3568 SoC PCIe host controller is based on the Synopsys DesignWare
> +  PCIe IP and thus inherits all the common properties defined in
> +  designware-pcie.txt.
> +
> +allOf:
> +  - $ref: /schemas/pci/pci-bus.yaml#
> +
> +# We need a select here so we don't match all nodes with 'snps,dw-pcie'
> +select:
> +  properties:
> +    compatible:
> +      contains:
> +        const: rockchip,rk3568-pcie
> +  required:
> +    - compatible
> +
> +properties:
> +  compatible:
> +    items:
> +      - const: rockchip,rk3568-pcie
> +      - const: snps,dw-pcie
> +
> +  reg:
> +    items:
> +      - description: Data Bus Interface (DBI) registers
> +      - description: Rockchip designed configuration registers

reg-names:
    items:
      - const: dbi
      - const: apb

rockchip->apb_base = devm_platform_ioremap_resource_byname(pdev, "apb");

config ???

> +
> +  clocks:
> +    items:
> +      - description: AHB clock for PCIe master
> +      - description: AHB clock for PCIe slave
> +      - description: AHB clock for PCIe dbi
> +      - description: APB clock for PCIe
> +      - description: Auxiliary clock for PCIe
> +
> +  clock-names:
> +    items:
> +      - const: aclk_mst
> +      - const: aclk_slv
> +      - const: aclk_dbi
> +      - const: pclk
> +      - const: aux
> +
> +  msi-map: true
> +
> +  num-lanes: true
> +
> +  phys:
> +    maxItems: 1
> +
> +  phy-names:
> +    const: pcie-phy
> +
> +  power-domains:
> +    maxItems: 1
> +
> +  ranges:

> +    maxItems: 3

if remove ??? config ???
<0x00000800 0x0 0x80000000 0x3 0x80000000 0x0 0x800000>
then maxItems: 2

> +

reset-gpios:
   maxItems: 1

rockchip->rst_gpio = devm_gpiod_get_optional(&pdev->dev, "reset",
GPIOD_OUT_HIGH);

> +  resets:
> +    maxItems: 1
> +
> +  reset-names:
> +    const: pipe

vpcie3v3-supply: true

rockchip->vpcie3v3 = devm_regulator_get_optional(dev, "vpcie3v3");

> +
> +required:
> +  - compatible
> +  - reg

> +  - reg-names

required but not defined above

> +  - clocks
> +  - clock-names
> +  - msi-map
> +  - num-lanes
> +  - phys
> +  - phy-names
> +  - power-domains
> +  - resets
> +  - reset-names
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |

> +    #include <dt-bindings/interrupt-controller/arm-gic.h>

no interrupts then no need for include defines ??

> +
> +    bus {
> +        #address-cells = <2>;
> +        #size-cells = <2>;
> +
> +        pcie3x2: pcie@fe280000 {
> +            compatible = "rockchip,rk3568-pcie", "snps,dw-pcie";
> +            reg = <0x3 0xc0800000 0x0 0x400000>,
> +                  <0x0 0xfe280000 0x0 0x10000>;

> +            reg-names = "pcie-dbi", "pcie-apb";

reg-names = "dbi", "apb";

> +            bus-range = <0x20 0x2f>;
> +            clocks = <&cru 143>, <&cru 144>,
> +                     <&cru 145>, <&cru 146>,
> +                     <&cru 147>;
> +            clock-names = "aclk_mst", "aclk_slv",
> +                          "aclk_dbi", "pclk",
> +                          "aux";
> +            device_type = "pci";
> +            linux,pci-domain = <2>;
> +            max-link-speed = <2>;
> +            msi-map = <0x2000 &its 0x2000 0x1000>;
> +            num-lanes = <2>;
> +            phys = <&pcie30phy>;
> +            phy-names = "pcie-phy";
> +            power-domains = <&power 15>;

> +            ranges = <0x00000800 0x0 0x80000000 0x3 0x80000000 0x0 0x800000>,

remove ??? config ???
<0x00000800 0x0 0x80000000 0x3 0x80000000 0x0 0x800000>

> +                     <0x81000000 0x0 0x80800000 0x3 0x80800000 0x0 0x100000>,
> +                     <0x83000000 0x0 0x80900000 0x3 0x80900000 0x0 0x3f700000>;
> +            resets = <&cru 193>;
> +            reset-names = "pipe";
> +            #address-cells = <3>;
> +            #size-cells = <2>;
> +        };
> +    };
> +...
> 


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

* Re: [PATCH v3 2/2] PCI: rockchip: add DesignWare based PCIe controller
  2021-01-25 15:53         ` Robin Murphy
@ 2021-01-25 18:45           ` Leon Romanovsky
  2021-01-26  2:34             ` xxm
  0 siblings, 1 reply; 15+ messages in thread
From: Leon Romanovsky @ 2021-01-25 18:45 UTC (permalink / raw)
  To: Robin Murphy
  Cc: xxm, devicetree, Lorenzo Pieralisi, Heiko Stuebner, linux-pci,
	Shawn Lin, linux-rockchip, robh+dt, Bjorn Helgaas, Johan Jonker

On Mon, Jan 25, 2021 at 03:53:38PM +0000, Robin Murphy wrote:
> On 2021-01-25 09:01, Leon Romanovsky wrote:
> > On Mon, Jan 25, 2021 at 02:40:10PM +0800, xxm wrote:
> > > Hi Leon,
> > >
> > > Thanks for your reply.
> > >
> > > 在 2021/1/25 13:48, Leon Romanovsky 写道:
> > > > On Mon, Jan 25, 2021 at 10:49:27AM +0800, Simon Xue wrote:
> > > > > pcie-dw-rockchip is based on DWC IP. But pcie-rockchip-host
> > > > > is Rockchip designed IP which is only used for RK3399. So all the following
> > > > > non-RK3399 SoCs should use this driver.
> > > > >
> > > > > Signed-off-by: Simon Xue <xxm@rock-chips.com>
> > > > > Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
> > > > > ---
> > > > >    drivers/pci/controller/dwc/Kconfig            |   9 +
> > > > >    drivers/pci/controller/dwc/Makefile           |   1 +
> > > > >    drivers/pci/controller/dwc/pcie-dw-rockchip.c | 286 ++++++++++++++++++
> > > > >    3 files changed, 296 insertions(+)
> > > > >    create mode 100644 drivers/pci/controller/dwc/pcie-dw-rockchip.c
> > > > >
> > > > > diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
> > > > > index 22c5529e9a65..aee408fe9283 100644
> > > > > --- a/drivers/pci/controller/dwc/Kconfig
> > > > > +++ b/drivers/pci/controller/dwc/Kconfig
> > > > > @@ -214,6 +214,15 @@ config PCIE_ARTPEC6_EP
> > > > >    	  Enables support for the PCIe controller in the ARTPEC-6 SoC to work in
> > > > >    	  endpoint mode. This uses the DesignWare core.
> > > > >
> > > > > +config PCIE_ROCKCHIP_DW_HOST
> > > > > +	bool "Rockchip DesignWare PCIe controller"
> > > > > +	select PCIE_DW
> > > > > +	select PCIE_DW_HOST
> > > > > +	depends on ARCH_ROCKCHIP || COMPILE_TEST
> > > > > +	depends on OF
> > > > > +	help
> > > > > +	  Enables support for the DW PCIe controller in the Rockchip SoC.
> > > > > +
> > > > >    config PCIE_INTEL_GW
> > > > >    	bool "Intel Gateway PCIe host controller support"
> > > > >    	depends on OF && (X86 || COMPILE_TEST)
> > > > > diff --git a/drivers/pci/controller/dwc/Makefile b/drivers/pci/controller/dwc/Makefile
> > > > > index a751553fa0db..30eef8e9ee8a 100644
> > > > > --- a/drivers/pci/controller/dwc/Makefile
> > > > > +++ b/drivers/pci/controller/dwc/Makefile
> > > > > @@ -13,6 +13,7 @@ obj-$(CONFIG_PCI_LAYERSCAPE_EP) += pci-layerscape-ep.o
> > > > >    obj-$(CONFIG_PCIE_QCOM) += pcie-qcom.o
> > > > >    obj-$(CONFIG_PCIE_ARMADA_8K) += pcie-armada8k.o
> > > > >    obj-$(CONFIG_PCIE_ARTPEC6) += pcie-artpec6.o
> > > > > +obj-$(CONFIG_PCIE_ROCKCHIP_DW_HOST) += pcie-dw-rockchip.o
> > > > >    obj-$(CONFIG_PCIE_INTEL_GW) += pcie-intel-gw.o
> > > > >    obj-$(CONFIG_PCIE_KIRIN) += pcie-kirin.o
> > > > >    obj-$(CONFIG_PCIE_HISI_STB) += pcie-histb.o
> > > > > diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> > > > > new file mode 100644
> > > > > index 000000000000..07f6d1cd5853
> > > > > --- /dev/null
> > > > > +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> > > > > @@ -0,0 +1,286 @@
> > > > > +// SPDX-License-Identifier: GPL-2.0
> > > > > +/*
> > > > > + * PCIe host controller driver for Rockchip SoCs
> > > > > + *
> > > > > + * Copyright (C) 2021 Rockchip Electronics Co., Ltd.
> > > > > + *		http://www.rock-chips.com
> > > > > + *
> > > > > + * Author: Simon Xue <xxm@rock-chips.com>
> > > > > + */
> > > > > +
> > > > > +#include <linux/clk.h>
> > > > > +#include <linux/gpio/consumer.h>
> > > > > +#include <linux/mfd/syscon.h>
> > > > > +#include <linux/module.h>
> > > > > +#include <linux/of_device.h>
> > > > > +#include <linux/phy/phy.h>
> > > > > +#include <linux/platform_device.h>
> > > > > +#include <linux/regmap.h>
> > > > > +#include <linux/reset.h>
> > > > > +
> > > > > +#include "pcie-designware.h"
> > > > > +
> > > > > +/*
> > > > > + * The upper 16 bits of PCIE_CLIENT_CONFIG are a write
> > > > > + * mask for the lower 16 bits.  This allows atomic updates
> > > > > + * of the register without locking.
> > > > > + */
> > > > This is correct only for the variables that naturally aligned, I imagine
> > > > that this is the case here and in the Linux, but better do not write comments
> > > > in the code that are not accurate.
> > >
> > > Ok, will remove.
> > > I wonder what it would be when outside the Linux? Could you share some information?
> >
> > The C standard says nothing about atomicity, integer assignment maybe atomic,
> > maybe it isn’t. There is no guarantee, plain integer assignment in C is non-atomic
> > by definition.
> >
> > The atomicity of u32 is very dependent on hardware vendor, memory model and compiler,
> > for example x86 and ARMs guarantee atomicity for u32. This is why I said that probably
> > here (Linux) it is ok and you are not alone in expecting lockless write.
>
> Huh? What do variables and the abstract machine of the C language
> environment have to do with the definition of *hardware MMIO registers*? We
> don't write to registers with plain integer assignment of u32, we use
> writel() (precisely in order to bypass that abstract C environment).
>
> I appreciate that the comment is not universally true if taken completely
> out of context, but I that's true of pretty much all comments ever. If
> someone really were trying to learn basic programming principles from random
> comments in Linux drivers, then it's already a bit late for us to try and
> save them from themselves.

So what? Does it mean that new code should have comments that are not
correct? As you can see from this conversation, even the author didn't
know what u32 isn’t guaranteed to be atomic, so yes, the comments should
be correct.

Thanks

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

* Re: [PATCH v3 2/2] PCI: rockchip: add DesignWare based PCIe controller
  2021-01-25 18:45           ` Leon Romanovsky
@ 2021-01-26  2:34             ` xxm
  0 siblings, 0 replies; 15+ messages in thread
From: xxm @ 2021-01-26  2:34 UTC (permalink / raw)
  To: Leon Romanovsky, Robin Murphy
  Cc: devicetree, Lorenzo Pieralisi, Heiko Stuebner, linux-pci,
	Shawn Lin, linux-rockchip, robh+dt, Bjorn Helgaas, Johan Jonker

Hi Leon,

Thanks for your reply.

在 2021/1/26 2:45, Leon Romanovsky 写道:
> On Mon, Jan 25, 2021 at 03:53:38PM +0000, Robin Murphy wrote:
>> On 2021-01-25 09:01, Leon Romanovsky wrote:
>>> On Mon, Jan 25, 2021 at 02:40:10PM +0800, xxm wrote:
>>>> Hi Leon,
>>>>
>>>> Thanks for your reply.
>>>>
>>>> 在 2021/1/25 13:48, Leon Romanovsky 写道:
>>>>> On Mon, Jan 25, 2021 at 10:49:27AM +0800, Simon Xue wrote:
>>>>>> pcie-dw-rockchip is based on DWC IP. But pcie-rockchip-host
>>>>>> is Rockchip designed IP which is only used for RK3399. So all the following
>>>>>> non-RK3399 SoCs should use this driver.
>>>>>>
>>>>>> Signed-off-by: Simon Xue <xxm@rock-chips.com>
>>>>>> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
>>>>>> ---
>>>>>>     drivers/pci/controller/dwc/Kconfig            |   9 +
>>>>>>     drivers/pci/controller/dwc/Makefile           |   1 +
>>>>>>     drivers/pci/controller/dwc/pcie-dw-rockchip.c | 286 ++++++++++++++++++
>>>>>>     3 files changed, 296 insertions(+)
>>>>>>     create mode 100644 drivers/pci/controller/dwc/pcie-dw-rockchip.c
>>>>>>
>>>>>> diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
>>>>>> index 22c5529e9a65..aee408fe9283 100644
>>>>>> --- a/drivers/pci/controller/dwc/Kconfig
>>>>>> +++ b/drivers/pci/controller/dwc/Kconfig
>>>>>> @@ -214,6 +214,15 @@ config PCIE_ARTPEC6_EP
>>>>>>     	  Enables support for the PCIe controller in the ARTPEC-6 SoC to work in
>>>>>>     	  endpoint mode. This uses the DesignWare core.
>>>>>>
>>>>>> +config PCIE_ROCKCHIP_DW_HOST
>>>>>> +	bool "Rockchip DesignWare PCIe controller"
>>>>>> +	select PCIE_DW
>>>>>> +	select PCIE_DW_HOST
>>>>>> +	depends on ARCH_ROCKCHIP || COMPILE_TEST
>>>>>> +	depends on OF
>>>>>> +	help
>>>>>> +	  Enables support for the DW PCIe controller in the Rockchip SoC.
>>>>>> +
>>>>>>     config PCIE_INTEL_GW
>>>>>>     	bool "Intel Gateway PCIe host controller support"
>>>>>>     	depends on OF && (X86 || COMPILE_TEST)
>>>>>> diff --git a/drivers/pci/controller/dwc/Makefile b/drivers/pci/controller/dwc/Makefile
>>>>>> index a751553fa0db..30eef8e9ee8a 100644
>>>>>> --- a/drivers/pci/controller/dwc/Makefile
>>>>>> +++ b/drivers/pci/controller/dwc/Makefile
>>>>>> @@ -13,6 +13,7 @@ obj-$(CONFIG_PCI_LAYERSCAPE_EP) += pci-layerscape-ep.o
>>>>>>     obj-$(CONFIG_PCIE_QCOM) += pcie-qcom.o
>>>>>>     obj-$(CONFIG_PCIE_ARMADA_8K) += pcie-armada8k.o
>>>>>>     obj-$(CONFIG_PCIE_ARTPEC6) += pcie-artpec6.o
>>>>>> +obj-$(CONFIG_PCIE_ROCKCHIP_DW_HOST) += pcie-dw-rockchip.o
>>>>>>     obj-$(CONFIG_PCIE_INTEL_GW) += pcie-intel-gw.o
>>>>>>     obj-$(CONFIG_PCIE_KIRIN) += pcie-kirin.o
>>>>>>     obj-$(CONFIG_PCIE_HISI_STB) += pcie-histb.o
>>>>>> diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
>>>>>> new file mode 100644
>>>>>> index 000000000000..07f6d1cd5853
>>>>>> --- /dev/null
>>>>>> +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
>>>>>> @@ -0,0 +1,286 @@
>>>>>> +// SPDX-License-Identifier: GPL-2.0
>>>>>> +/*
>>>>>> + * PCIe host controller driver for Rockchip SoCs
>>>>>> + *
>>>>>> + * Copyright (C) 2021 Rockchip Electronics Co., Ltd.
>>>>>> + *		http://www.rock-chips.com
>>>>>> + *
>>>>>> + * Author: Simon Xue <xxm@rock-chips.com>
>>>>>> + */
>>>>>> +
>>>>>> +#include <linux/clk.h>
>>>>>> +#include <linux/gpio/consumer.h>
>>>>>> +#include <linux/mfd/syscon.h>
>>>>>> +#include <linux/module.h>
>>>>>> +#include <linux/of_device.h>
>>>>>> +#include <linux/phy/phy.h>
>>>>>> +#include <linux/platform_device.h>
>>>>>> +#include <linux/regmap.h>
>>>>>> +#include <linux/reset.h>
>>>>>> +
>>>>>> +#include "pcie-designware.h"
>>>>>> +
>>>>>> +/*
>>>>>> + * The upper 16 bits of PCIE_CLIENT_CONFIG are a write
>>>>>> + * mask for the lower 16 bits.  This allows atomic updates
>>>>>> + * of the register without locking.
>>>>>> + */
>>>>> This is correct only for the variables that naturally aligned, I imagine
>>>>> that this is the case here and in the Linux, but better do not write comments
>>>>> in the code that are not accurate.
>>>> Ok, will remove.
>>>> I wonder what it would be when outside the Linux? Could you share some information?
>>> The C standard says nothing about atomicity, integer assignment maybe atomic,
>>> maybe it isn’t. There is no guarantee, plain integer assignment in C is non-atomic
>>> by definition.
>>>
>>> The atomicity of u32 is very dependent on hardware vendor, memory model and compiler,
>>> for example x86 and ARMs guarantee atomicity for u32. This is why I said that probably
>>> here (Linux) it is ok and you are not alone in expecting lockless write.
>> Huh? What do variables and the abstract machine of the C language
>> environment have to do with the definition of *hardware MMIO registers*? We
>> don't write to registers with plain integer assignment of u32, we use
>> writel() (precisely in order to bypass that abstract C environment).
>>
>> I appreciate that the comment is not universally true if taken completely
>> out of context, but I that's true of pretty much all comments ever. If
>> someone really were trying to learn basic programming principles from random
>> comments in Linux drivers, then it's already a bit late for us to try and
>> save them from themselves.
> So what? Does it mean that new code should have comments that are not
> correct? As you can see from this conversation, even the author didn't
> know what u32 isn’t guaranteed to be atomic, so yes, the comments should
> be correct.

What I do know is writel() will do the right things(like mem barrier, 
atomic...) to update the registers correctly

in "ARM + Linux" platform. But I have no idear if out of  this specific 
platform, so I asked for more information to learn.

Anyway, I will keep the first part of comment to illustrate how to use 
PCIE_CLIENT_REGISTER, and remove the "atomic" part.

> Thanks
>
>



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

* Re: [PATCH v3 1/2] dt-bindings: rockchip: Add DesignWare based PCIe controller【请注意,邮件由robherring2@gmail.com代发】
  2021-01-25 15:26 ` Rob Herring
@ 2021-01-26  2:44   ` xxm
  2021-01-26 14:48     ` Rob Herring
  0 siblings, 1 reply; 15+ messages in thread
From: xxm @ 2021-01-26  2:44 UTC (permalink / raw)
  To: Rob Herring
  Cc: Bjorn Helgaas, Lorenzo Pieralisi, linux-pci, linux-rockchip,
	devicetree, Johan Jonker, Heiko Stuebner

Hi Rob,

Thanks for reply.

在 2021/1/25 23:26, Rob Herring 写道:
> On Mon, Jan 25, 2021 at 10:48:24AM +0800, Simon Xue wrote:
>> Document DT bindings for PCIe controller found on Rockchip SoC.
>>
>> Signed-off-by: Simon Xue <xxm@rock-chips.com>
>> ---
>>   .../bindings/pci/rockchip-dw-pcie.yaml        | 133 ++++++++++++++++++
>>   1 file changed, 133 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
>> new file mode 100644
>> index 000000000000..24ea42203c14
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
>> @@ -0,0 +1,133 @@
>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/pci/rockchip-dw-pcie.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: DesignWare based PCIe RC controller on Rockchip SoCs
>> +
>> +maintainers:
>> +  - Shawn Lin <shawn.lin@rock-chips.com>
>> +  - Simon Xue <xxm@rock-chips.com>
>> +  - Heiko Stuebner <heiko@sntech.de>
>> +
>> +description: |+
>> +  RK3568 SoC PCIe host controller is based on the Synopsys DesignWare
>> +  PCIe IP and thus inherits all the common properties defined in
>> +  designware-pcie.txt.
>> +
>> +allOf:
>> +  - $ref: /schemas/pci/pci-bus.yaml#
>> +
>> +# We need a select here so we don't match all nodes with 'snps,dw-pcie'
>> +select:
>> +  properties:
>> +    compatible:
>> +      contains:
>> +        const: rockchip,rk3568-pcie
>> +  required:
>> +    - compatible
>> +
>> +properties:
>> +  compatible:
>> +    items:
>> +      - const: rockchip,rk3568-pcie
>> +      - const: snps,dw-pcie
>> +
>> +  reg:
>> +    items:
>> +      - description: Data Bus Interface (DBI) registers
>> +      - description: Rockchip designed configuration registers
>> +
>> +  clocks:
>> +    items:
>> +      - description: AHB clock for PCIe master
>> +      - description: AHB clock for PCIe slave
>> +      - description: AHB clock for PCIe dbi
>> +      - description: APB clock for PCIe
>> +      - description: Auxiliary clock for PCIe
>> +
>> +  clock-names:
>> +    items:
>> +      - const: aclk_mst
>> +      - const: aclk_slv
>> +      - const: aclk_dbi
>> +      - const: pclk
>> +      - const: aux
>> +
>> +  msi-map: true
>> +
>> +  num-lanes: true
>> +
>> +  phys:
>> +    maxItems: 1
>> +
>> +  phy-names:
>> +    const: pcie-phy
>> +
>> +  power-domains:
>> +    maxItems: 1
>> +
>> +  ranges:
>> +    maxItems: 3
>> +
>> +  resets:
>> +    maxItems: 1
>> +
>> +  reset-names:
>> +    const: pipe
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - reg-names
>> +  - clocks
>> +  - clock-names
>> +  - msi-map
>> +  - num-lanes
>> +  - phys
>> +  - phy-names
>> +  - power-domains
>> +  - resets
>> +  - reset-names
>> +
>> +unevaluatedProperties: false
>> +
>> +examples:
>> +  - |
>> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
>> +
>> +    bus {
>> +        #address-cells = <2>;
>> +        #size-cells = <2>;
>> +
>> +        pcie3x2: pcie@fe280000 {
>> +            compatible = "rockchip,rk3568-pcie", "snps,dw-pcie";
>> +            reg = <0x3 0xc0800000 0x0 0x400000>,
>> +                  <0x0 0xfe280000 0x0 0x10000>;
>> +            reg-names = "pcie-dbi", "pcie-apb";
> I believe I already said use 'dbi'. The DBI is also not 4MB. The config
> space goes here too, not in 'ranges'.

Sorry for missing  update in yaml.

I think yaml is used to describe the resources of specific SoC, it 
reserves 4MB for DBI on Rockchip SoC.

So, I think assign 4MB here is reasonable.

>> +            bus-range = <0x20 0x2f>;
>> +            clocks = <&cru 143>, <&cru 144>,
>> +                     <&cru 145>, <&cru 146>,
>> +                     <&cru 147>;
>> +            clock-names = "aclk_mst", "aclk_slv",
>> +                          "aclk_dbi", "pclk",
>> +                          "aux";
>> +            device_type = "pci";
>> +            linux,pci-domain = <2>;
>> +            max-link-speed = <2>;
>> +            msi-map = <0x2000 &its 0x2000 0x1000>;
>> +            num-lanes = <2>;
>> +            phys = <&pcie30phy>;
>> +            phy-names = "pcie-phy";
>> +            power-domains = <&power 15>;
>> +            ranges = <0x00000800 0x0 0x80000000 0x3 0x80000000 0x0 0x800000>,
>> +                     <0x81000000 0x0 0x80800000 0x3 0x80800000 0x0 0x100000>,
>> +                     <0x83000000 0x0 0x80900000 0x3 0x80900000 0x0 0x3f700000>;
>> +            resets = <&cru 193>;
>> +            reset-names = "pipe";
>> +            #address-cells = <3>;
>> +            #size-cells = <2>;
>> +        };
>> +    };
>> +...
>> -- 
>> 2.25.1
>>
>>
>>
>
>



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

* Re: [PATCH v3 1/2] dt-bindings: rockchip: Add DesignWare based PCIe controller【请注意,邮件由robherring2@gmail.com代发】
  2021-01-26  2:44   ` [PATCH v3 1/2] dt-bindings: rockchip: Add DesignWare based PCIe controller【请注意,邮件由robherring2@gmail.com代发】 xxm
@ 2021-01-26 14:48     ` Rob Herring
  0 siblings, 0 replies; 15+ messages in thread
From: Rob Herring @ 2021-01-26 14:48 UTC (permalink / raw)
  To: xxm
  Cc: Bjorn Helgaas, Lorenzo Pieralisi, PCI,
	open list:ARM/Rockchip SoC...,
	devicetree, Johan Jonker, Heiko Stuebner

On Mon, Jan 25, 2021 at 8:44 PM xxm <xxm@rock-chips.com> wrote:
>
> Hi Rob,
>
> Thanks for reply.
>
> 在 2021/1/25 23:26, Rob Herring 写道:
> > On Mon, Jan 25, 2021 at 10:48:24AM +0800, Simon Xue wrote:
> >> Document DT bindings for PCIe controller found on Rockchip SoC.
> >>
> >> Signed-off-by: Simon Xue <xxm@rock-chips.com>
> >> ---
> >>   .../bindings/pci/rockchip-dw-pcie.yaml        | 133 ++++++++++++++++++
> >>   1 file changed, 133 insertions(+)
> >>   create mode 100644 Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
> >>
> >> diff --git a/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
> >> new file mode 100644
> >> index 000000000000..24ea42203c14
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
> >> @@ -0,0 +1,133 @@
> >> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> >> +%YAML 1.2
> >> +---
> >> +$id: http://devicetree.org/schemas/pci/rockchip-dw-pcie.yaml#
> >> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >> +
> >> +title: DesignWare based PCIe RC controller on Rockchip SoCs
> >> +
> >> +maintainers:
> >> +  - Shawn Lin <shawn.lin@rock-chips.com>
> >> +  - Simon Xue <xxm@rock-chips.com>
> >> +  - Heiko Stuebner <heiko@sntech.de>
> >> +
> >> +description: |+
> >> +  RK3568 SoC PCIe host controller is based on the Synopsys DesignWare
> >> +  PCIe IP and thus inherits all the common properties defined in
> >> +  designware-pcie.txt.
> >> +
> >> +allOf:
> >> +  - $ref: /schemas/pci/pci-bus.yaml#
> >> +
> >> +# We need a select here so we don't match all nodes with 'snps,dw-pcie'
> >> +select:
> >> +  properties:
> >> +    compatible:
> >> +      contains:
> >> +        const: rockchip,rk3568-pcie
> >> +  required:
> >> +    - compatible
> >> +
> >> +properties:
> >> +  compatible:
> >> +    items:
> >> +      - const: rockchip,rk3568-pcie
> >> +      - const: snps,dw-pcie
> >> +
> >> +  reg:
> >> +    items:
> >> +      - description: Data Bus Interface (DBI) registers
> >> +      - description: Rockchip designed configuration registers
> >> +
> >> +  clocks:
> >> +    items:
> >> +      - description: AHB clock for PCIe master
> >> +      - description: AHB clock for PCIe slave
> >> +      - description: AHB clock for PCIe dbi
> >> +      - description: APB clock for PCIe
> >> +      - description: Auxiliary clock for PCIe
> >> +
> >> +  clock-names:
> >> +    items:
> >> +      - const: aclk_mst
> >> +      - const: aclk_slv
> >> +      - const: aclk_dbi
> >> +      - const: pclk
> >> +      - const: aux
> >> +
> >> +  msi-map: true
> >> +
> >> +  num-lanes: true
> >> +
> >> +  phys:
> >> +    maxItems: 1
> >> +
> >> +  phy-names:
> >> +    const: pcie-phy
> >> +
> >> +  power-domains:
> >> +    maxItems: 1
> >> +
> >> +  ranges:
> >> +    maxItems: 3
> >> +
> >> +  resets:
> >> +    maxItems: 1
> >> +
> >> +  reset-names:
> >> +    const: pipe
> >> +
> >> +required:
> >> +  - compatible
> >> +  - reg
> >> +  - reg-names
> >> +  - clocks
> >> +  - clock-names
> >> +  - msi-map
> >> +  - num-lanes
> >> +  - phys
> >> +  - phy-names
> >> +  - power-domains
> >> +  - resets
> >> +  - reset-names
> >> +
> >> +unevaluatedProperties: false
> >> +
> >> +examples:
> >> +  - |
> >> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> >> +
> >> +    bus {
> >> +        #address-cells = <2>;
> >> +        #size-cells = <2>;
> >> +
> >> +        pcie3x2: pcie@fe280000 {
> >> +            compatible = "rockchip,rk3568-pcie", "snps,dw-pcie";
> >> +            reg = <0x3 0xc0800000 0x0 0x400000>,
> >> +                  <0x0 0xfe280000 0x0 0x10000>;
> >> +            reg-names = "pcie-dbi", "pcie-apb";
> > I believe I already said use 'dbi'. The DBI is also not 4MB. The config
> > space goes here too, not in 'ranges'.
>
> Sorry for missing  update in yaml.
>
> I think yaml is used to describe the resources of specific SoC, it
> reserves 4MB for DBI on Rockchip SoC.
>
> So, I think assign 4MB here is reasonable.

Not if there's nothing there. Otherwise you are wasting almost 4MB of
virtual space. Doesn't matter so much on 64-bit, but for 32-bit it
really does.

Rob

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

* Re: [PATCH v3 2/2] PCI: rockchip: add DesignWare based PCIe controller
  2021-01-25  9:01       ` Leon Romanovsky
  2021-01-25 15:53         ` Robin Murphy
@ 2021-01-26 14:52         ` Rob Herring
  2021-01-26 15:25           ` Leon Romanovsky
  1 sibling, 1 reply; 15+ messages in thread
From: Rob Herring @ 2021-01-26 14:52 UTC (permalink / raw)
  To: Leon Romanovsky
  Cc: xxm, Bjorn Helgaas, Lorenzo Pieralisi, PCI,
	open list:ARM/Rockchip SoC...,
	devicetree, Johan Jonker, Heiko Stuebner, Shawn Lin

On Mon, Jan 25, 2021 at 3:01 AM Leon Romanovsky <leon@kernel.org> wrote:
>
> On Mon, Jan 25, 2021 at 02:40:10PM +0800, xxm wrote:
> > Hi Leon,
> >
> > Thanks for your reply.
> >
> > 在 2021/1/25 13:48, Leon Romanovsky 写道:
> > > On Mon, Jan 25, 2021 at 10:49:27AM +0800, Simon Xue wrote:
> > > > pcie-dw-rockchip is based on DWC IP. But pcie-rockchip-host
> > > > is Rockchip designed IP which is only used for RK3399. So all the following
> > > > non-RK3399 SoCs should use this driver.
> > > >
> > > > Signed-off-by: Simon Xue <xxm@rock-chips.com>
> > > > Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
> > > > ---
> > > >   drivers/pci/controller/dwc/Kconfig            |   9 +
> > > >   drivers/pci/controller/dwc/Makefile           |   1 +
> > > >   drivers/pci/controller/dwc/pcie-dw-rockchip.c | 286 ++++++++++++++++++
> > > >   3 files changed, 296 insertions(+)
> > > >   create mode 100644 drivers/pci/controller/dwc/pcie-dw-rockchip.c
> > > >
> > > > diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
> > > > index 22c5529e9a65..aee408fe9283 100644
> > > > --- a/drivers/pci/controller/dwc/Kconfig
> > > > +++ b/drivers/pci/controller/dwc/Kconfig
> > > > @@ -214,6 +214,15 @@ config PCIE_ARTPEC6_EP
> > > >             Enables support for the PCIe controller in the ARTPEC-6 SoC to work in
> > > >             endpoint mode. This uses the DesignWare core.
> > > >
> > > > +config PCIE_ROCKCHIP_DW_HOST
> > > > + bool "Rockchip DesignWare PCIe controller"
> > > > + select PCIE_DW
> > > > + select PCIE_DW_HOST
> > > > + depends on ARCH_ROCKCHIP || COMPILE_TEST
> > > > + depends on OF
> > > > + help
> > > > +   Enables support for the DW PCIe controller in the Rockchip SoC.
> > > > +
> > > >   config PCIE_INTEL_GW
> > > >           bool "Intel Gateway PCIe host controller support"
> > > >           depends on OF && (X86 || COMPILE_TEST)
> > > > diff --git a/drivers/pci/controller/dwc/Makefile b/drivers/pci/controller/dwc/Makefile
> > > > index a751553fa0db..30eef8e9ee8a 100644
> > > > --- a/drivers/pci/controller/dwc/Makefile
> > > > +++ b/drivers/pci/controller/dwc/Makefile
> > > > @@ -13,6 +13,7 @@ obj-$(CONFIG_PCI_LAYERSCAPE_EP) += pci-layerscape-ep.o
> > > >   obj-$(CONFIG_PCIE_QCOM) += pcie-qcom.o
> > > >   obj-$(CONFIG_PCIE_ARMADA_8K) += pcie-armada8k.o
> > > >   obj-$(CONFIG_PCIE_ARTPEC6) += pcie-artpec6.o
> > > > +obj-$(CONFIG_PCIE_ROCKCHIP_DW_HOST) += pcie-dw-rockchip.o
> > > >   obj-$(CONFIG_PCIE_INTEL_GW) += pcie-intel-gw.o
> > > >   obj-$(CONFIG_PCIE_KIRIN) += pcie-kirin.o
> > > >   obj-$(CONFIG_PCIE_HISI_STB) += pcie-histb.o
> > > > diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> > > > new file mode 100644
> > > > index 000000000000..07f6d1cd5853
> > > > --- /dev/null
> > > > +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> > > > @@ -0,0 +1,286 @@
> > > > +// SPDX-License-Identifier: GPL-2.0
> > > > +/*
> > > > + * PCIe host controller driver for Rockchip SoCs
> > > > + *
> > > > + * Copyright (C) 2021 Rockchip Electronics Co., Ltd.
> > > > + *               http://www.rock-chips.com
> > > > + *
> > > > + * Author: Simon Xue <xxm@rock-chips.com>
> > > > + */
> > > > +
> > > > +#include <linux/clk.h>
> > > > +#include <linux/gpio/consumer.h>
> > > > +#include <linux/mfd/syscon.h>
> > > > +#include <linux/module.h>
> > > > +#include <linux/of_device.h>
> > > > +#include <linux/phy/phy.h>
> > > > +#include <linux/platform_device.h>
> > > > +#include <linux/regmap.h>
> > > > +#include <linux/reset.h>
> > > > +
> > > > +#include "pcie-designware.h"
> > > > +
> > > > +/*
> > > > + * The upper 16 bits of PCIE_CLIENT_CONFIG are a write
> > > > + * mask for the lower 16 bits.  This allows atomic updates
> > > > + * of the register without locking.
> > > > + */
> > > This is correct only for the variables that naturally aligned, I imagine
> > > that this is the case here and in the Linux, but better do not write comments
> > > in the code that are not accurate.
> >
> > Ok, will remove.
> > I wonder what it would be when outside the Linux? Could you share some information?
>
> The C standard says nothing about atomicity, integer assignment maybe atomic,
> maybe it isn’t. There is no guarantee, plain integer assignment in C is non-atomic
> by definition.
>
> The atomicity of u32 is very dependent on hardware vendor, memory model and compiler,
> for example x86 and ARMs guarantee atomicity for u32. This is why I said that probably
> here (Linux) it is ok and you are not alone in expecting lockless write.

But this is a mmio register accessed thru writel() which does have all
those guarantees.

Rob

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

* Re: [PATCH v3 2/2] PCI: rockchip: add DesignWare based PCIe controller
  2021-01-26 14:52         ` Rob Herring
@ 2021-01-26 15:25           ` Leon Romanovsky
  0 siblings, 0 replies; 15+ messages in thread
From: Leon Romanovsky @ 2021-01-26 15:25 UTC (permalink / raw)
  To: Rob Herring
  Cc: xxm, Bjorn Helgaas, Lorenzo Pieralisi, PCI,
	open list:ARM/Rockchip SoC...,
	devicetree, Johan Jonker, Heiko Stuebner, Shawn Lin

On Tue, Jan 26, 2021 at 08:52:31AM -0600, Rob Herring wrote:
> On Mon, Jan 25, 2021 at 3:01 AM Leon Romanovsky <leon@kernel.org> wrote:
> >
> > On Mon, Jan 25, 2021 at 02:40:10PM +0800, xxm wrote:
> > > Hi Leon,
> > >
> > > Thanks for your reply.
> > >
> > > 在 2021/1/25 13:48, Leon Romanovsky 写道:
> > > > On Mon, Jan 25, 2021 at 10:49:27AM +0800, Simon Xue wrote:
> > > > > pcie-dw-rockchip is based on DWC IP. But pcie-rockchip-host
> > > > > is Rockchip designed IP which is only used for RK3399. So all the following
> > > > > non-RK3399 SoCs should use this driver.
> > > > >
> > > > > Signed-off-by: Simon Xue <xxm@rock-chips.com>
> > > > > Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
> > > > > ---
> > > > >   drivers/pci/controller/dwc/Kconfig            |   9 +
> > > > >   drivers/pci/controller/dwc/Makefile           |   1 +
> > > > >   drivers/pci/controller/dwc/pcie-dw-rockchip.c | 286 ++++++++++++++++++
> > > > >   3 files changed, 296 insertions(+)
> > > > >   create mode 100644 drivers/pci/controller/dwc/pcie-dw-rockchip.c
> > > > >
> > > > > diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
> > > > > index 22c5529e9a65..aee408fe9283 100644
> > > > > --- a/drivers/pci/controller/dwc/Kconfig
> > > > > +++ b/drivers/pci/controller/dwc/Kconfig
> > > > > @@ -214,6 +214,15 @@ config PCIE_ARTPEC6_EP
> > > > >             Enables support for the PCIe controller in the ARTPEC-6 SoC to work in
> > > > >             endpoint mode. This uses the DesignWare core.
> > > > >
> > > > > +config PCIE_ROCKCHIP_DW_HOST
> > > > > + bool "Rockchip DesignWare PCIe controller"
> > > > > + select PCIE_DW
> > > > > + select PCIE_DW_HOST
> > > > > + depends on ARCH_ROCKCHIP || COMPILE_TEST
> > > > > + depends on OF
> > > > > + help
> > > > > +   Enables support for the DW PCIe controller in the Rockchip SoC.
> > > > > +
> > > > >   config PCIE_INTEL_GW
> > > > >           bool "Intel Gateway PCIe host controller support"
> > > > >           depends on OF && (X86 || COMPILE_TEST)
> > > > > diff --git a/drivers/pci/controller/dwc/Makefile b/drivers/pci/controller/dwc/Makefile
> > > > > index a751553fa0db..30eef8e9ee8a 100644
> > > > > --- a/drivers/pci/controller/dwc/Makefile
> > > > > +++ b/drivers/pci/controller/dwc/Makefile
> > > > > @@ -13,6 +13,7 @@ obj-$(CONFIG_PCI_LAYERSCAPE_EP) += pci-layerscape-ep.o
> > > > >   obj-$(CONFIG_PCIE_QCOM) += pcie-qcom.o
> > > > >   obj-$(CONFIG_PCIE_ARMADA_8K) += pcie-armada8k.o
> > > > >   obj-$(CONFIG_PCIE_ARTPEC6) += pcie-artpec6.o
> > > > > +obj-$(CONFIG_PCIE_ROCKCHIP_DW_HOST) += pcie-dw-rockchip.o
> > > > >   obj-$(CONFIG_PCIE_INTEL_GW) += pcie-intel-gw.o
> > > > >   obj-$(CONFIG_PCIE_KIRIN) += pcie-kirin.o
> > > > >   obj-$(CONFIG_PCIE_HISI_STB) += pcie-histb.o
> > > > > diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> > > > > new file mode 100644
> > > > > index 000000000000..07f6d1cd5853
> > > > > --- /dev/null
> > > > > +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> > > > > @@ -0,0 +1,286 @@
> > > > > +// SPDX-License-Identifier: GPL-2.0
> > > > > +/*
> > > > > + * PCIe host controller driver for Rockchip SoCs
> > > > > + *
> > > > > + * Copyright (C) 2021 Rockchip Electronics Co., Ltd.
> > > > > + *               http://www.rock-chips.com
> > > > > + *
> > > > > + * Author: Simon Xue <xxm@rock-chips.com>
> > > > > + */
> > > > > +
> > > > > +#include <linux/clk.h>
> > > > > +#include <linux/gpio/consumer.h>
> > > > > +#include <linux/mfd/syscon.h>
> > > > > +#include <linux/module.h>
> > > > > +#include <linux/of_device.h>
> > > > > +#include <linux/phy/phy.h>
> > > > > +#include <linux/platform_device.h>
> > > > > +#include <linux/regmap.h>
> > > > > +#include <linux/reset.h>
> > > > > +
> > > > > +#include "pcie-designware.h"
> > > > > +
> > > > > +/*
> > > > > + * The upper 16 bits of PCIE_CLIENT_CONFIG are a write
> > > > > + * mask for the lower 16 bits.  This allows atomic updates
> > > > > + * of the register without locking.
> > > > > + */
> > > > This is correct only for the variables that naturally aligned, I imagine
> > > > that this is the case here and in the Linux, but better do not write comments
> > > > in the code that are not accurate.
> > >
> > > Ok, will remove.
> > > I wonder what it would be when outside the Linux? Could you share some information?
> >
> > The C standard says nothing about atomicity, integer assignment maybe atomic,
> > maybe it isn’t. There is no guarantee, plain integer assignment in C is non-atomic
> > by definition.
> >
> > The atomicity of u32 is very dependent on hardware vendor, memory model and compiler,
> > for example x86 and ARMs guarantee atomicity for u32. This is why I said that probably
> > here (Linux) it is ok and you are not alone in expecting lockless write.
>
> But this is a mmio register accessed thru writel() which does have all
> those guarantees.

The author didn't write "The writel() guarantees atomic updates
without need of locking".

Anyway, this is not important.

Thanks

>
> Rob

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

end of thread, other threads:[~2021-01-26 19:36 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-25  2:48 [PATCH v3 1/2] dt-bindings: rockchip: Add DesignWare based PCIe controller Simon Xue
2021-01-25  2:49 ` [PATCH v3 2/2] PCI: rockchip: add " Simon Xue
2021-01-25  5:48   ` Leon Romanovsky
2021-01-25  6:40     ` xxm
2021-01-25  9:01       ` Leon Romanovsky
2021-01-25 15:53         ` Robin Murphy
2021-01-25 18:45           ` Leon Romanovsky
2021-01-26  2:34             ` xxm
2021-01-26 14:52         ` Rob Herring
2021-01-26 15:25           ` Leon Romanovsky
2021-01-25 14:51 ` [PATCH v3 1/2] dt-bindings: rockchip: Add " Rob Herring
2021-01-25 15:26 ` Rob Herring
2021-01-26  2:44   ` [PATCH v3 1/2] dt-bindings: rockchip: Add DesignWare based PCIe controller【请注意,邮件由robherring2@gmail.com代发】 xxm
2021-01-26 14:48     ` Rob Herring
2021-01-25 15:55 ` [PATCH v3 1/2] dt-bindings: rockchip: Add DesignWare based PCIe controller Johan Jonker

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).