linux-pci.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v6 0/6] Add SiFive FU740 PCIe host controller driver support
@ 2021-05-04 10:59 Greentime Hu
  2021-05-04 10:59 ` [PATCH v6 1/6] clk: sifive: Add pcie_aux clock in prci driver for PCIe driver Greentime Hu
                   ` (6 more replies)
  0 siblings, 7 replies; 17+ messages in thread
From: Greentime Hu @ 2021-05-04 10:59 UTC (permalink / raw)
  To: greentime.hu, paul.walmsley, hes, erik.danie, zong.li, bhelgaas,
	robh+dt, aou, mturquette, sboyd, lorenzo.pieralisi, p.zabel,
	alex.dewar90, khilman, hayashi.kunihiko, vidyas, jh80.chung,
	linux-pci, devicetree, linux-riscv, linux-kernel, linux-clk,
	helgaas

This patchset includes SiFive FU740 PCIe host controller driver. We also
add pcie_aux clock and pcie_power_on_reset controller to prci driver for
PCIe driver to use it.

This is tested with e1000e: Intel(R) PRO/1000 Network Card, AMD Radeon R5
230 graphics card and SP M.2 PCIe Gen 3 SSD in SiFive Unmatched based on
v5.11 Linux kernel.

Changes in v6:
 - Fix build error when CONFIG_GPIOLIB disabled

Changes in v5:
 - Fix typo in comments
 - Keep comments style consistent
 - Refine some error handling codes
 - Remove unneeded header file including
 - Merge fu740_pcie_ltssm_enable implementation to fu740_pcie_start_link

Changes in v4:
 - Fix Wunused-but-set-variable warning in prci driver

Changes in v3:
 - Remove items that has been defined
 - Refine format of sifive,fu740-pcie.yaml
 - Replace perstn-gpios with the common one
 - Change DBI mapping space to 2GB from 4GB
 - Refine drivers/reset/Kconfig

Changes in v2:
 - Refine codes based on reviewers' feedback
 - Remove define and use the common one
 - Replace __raw_writel with writel_relaxed
 - Split fu740_phyregreadwrite to write function
 - Use readl_poll_timeout in stead of while loop checking
 - Use dwc common codes
 - Use gpio descriptors and the gpiod_* api.
 - Replace devm_ioremap_resource with devm_platform_ioremap_resource_byname
 - Replace devm_reset_control_get with devm_reset_control_get_exclusive
 - Add more comments for delay and sleep
 - Remove "phy ? x : y" expressions
 - Refine code logic to remove possible infinite loop
 - Replace magic number with meaningful define
 - Remove fu740_pcie_pm_ops
 - Use builtin_platform_driver

Greentime Hu (5):
  clk: sifive: Add pcie_aux clock in prci driver for PCIe driver
  clk: sifive: Use reset-simple in prci driver for PCIe driver
  MAINTAINERS: Add maintainers for SiFive FU740 PCIe driver
  dt-bindings: PCI: Add SiFive FU740 PCIe host controller
  riscv: dts: Add PCIe support for the SiFive FU740-C000 SoC

Paul Walmsley (1):
  PCI: fu740: Add SiFive FU740 PCIe host controller driver

 .../bindings/pci/sifive,fu740-pcie.yaml       | 113 +++++++
 MAINTAINERS                                   |   8 +
 arch/riscv/boot/dts/sifive/fu740-c000.dtsi    |  33 ++
 drivers/clk/sifive/Kconfig                    |   2 +
 drivers/clk/sifive/fu740-prci.c               |  11 +
 drivers/clk/sifive/fu740-prci.h               |   2 +-
 drivers/clk/sifive/sifive-prci.c              |  54 +++
 drivers/clk/sifive/sifive-prci.h              |  13 +
 drivers/pci/controller/dwc/Kconfig            |  10 +
 drivers/pci/controller/dwc/Makefile           |   1 +
 drivers/pci/controller/dwc/pcie-fu740.c       | 309 ++++++++++++++++++
 drivers/reset/Kconfig                         |   1 +
 include/dt-bindings/clock/sifive-fu740-prci.h |   1 +
 13 files changed, 557 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/pci/sifive,fu740-pcie.yaml
 create mode 100644 drivers/pci/controller/dwc/pcie-fu740.c

-- 
2.31.1


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

* [PATCH v6 1/6] clk: sifive: Add pcie_aux clock in prci driver for PCIe driver
  2021-05-04 10:59 [PATCH v6 0/6] Add SiFive FU740 PCIe host controller driver support Greentime Hu
@ 2021-05-04 10:59 ` Greentime Hu
  2021-05-04 12:24   ` Leon Romanovsky
  2021-05-04 10:59 ` [PATCH v6 2/6] clk: sifive: Use reset-simple " Greentime Hu
                   ` (5 subsequent siblings)
  6 siblings, 1 reply; 17+ messages in thread
From: Greentime Hu @ 2021-05-04 10:59 UTC (permalink / raw)
  To: greentime.hu, paul.walmsley, hes, erik.danie, zong.li, bhelgaas,
	robh+dt, aou, mturquette, sboyd, lorenzo.pieralisi, p.zabel,
	alex.dewar90, khilman, hayashi.kunihiko, vidyas, jh80.chung,
	linux-pci, devicetree, linux-riscv, linux-kernel, linux-clk,
	helgaas

We add pcie_aux clock in this patch so that pcie driver can use
clk_prepare_enable() and clk_disable_unprepare() to enable and disable
pcie_aux clock.

Signed-off-by: Greentime Hu <greentime.hu@sifive.com>
Acked-by: Stephen Boyd <sboyd@kernel.org>
---
 drivers/clk/sifive/fu740-prci.c               | 11 +++++
 drivers/clk/sifive/fu740-prci.h               |  2 +-
 drivers/clk/sifive/sifive-prci.c              | 41 +++++++++++++++++++
 drivers/clk/sifive/sifive-prci.h              |  9 ++++
 include/dt-bindings/clock/sifive-fu740-prci.h |  1 +
 5 files changed, 63 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/sifive/fu740-prci.c b/drivers/clk/sifive/fu740-prci.c
index 764d1097aa51..53f6e00a03b9 100644
--- a/drivers/clk/sifive/fu740-prci.c
+++ b/drivers/clk/sifive/fu740-prci.c
@@ -72,6 +72,12 @@ static const struct clk_ops sifive_fu740_prci_hfpclkplldiv_clk_ops = {
 	.recalc_rate = sifive_prci_hfpclkplldiv_recalc_rate,
 };
 
+static const struct clk_ops sifive_fu740_prci_pcie_aux_clk_ops = {
+	.enable = sifive_prci_pcie_aux_clock_enable,
+	.disable = sifive_prci_pcie_aux_clock_disable,
+	.is_enabled = sifive_prci_pcie_aux_clock_is_enabled,
+};
+
 /* List of clock controls provided by the PRCI */
 struct __prci_clock __prci_init_clocks_fu740[] = {
 	[PRCI_CLK_COREPLL] = {
@@ -120,4 +126,9 @@ struct __prci_clock __prci_init_clocks_fu740[] = {
 		.parent_name = "hfpclkpll",
 		.ops = &sifive_fu740_prci_hfpclkplldiv_clk_ops,
 	},
+	[PRCI_CLK_PCIE_AUX] = {
+		.name = "pcie_aux",
+		.parent_name = "hfclk",
+		.ops = &sifive_fu740_prci_pcie_aux_clk_ops,
+	},
 };
diff --git a/drivers/clk/sifive/fu740-prci.h b/drivers/clk/sifive/fu740-prci.h
index 13ef971f7764..511a0bf7ba2b 100644
--- a/drivers/clk/sifive/fu740-prci.h
+++ b/drivers/clk/sifive/fu740-prci.h
@@ -9,7 +9,7 @@
 
 #include "sifive-prci.h"
 
-#define NUM_CLOCK_FU740	8
+#define NUM_CLOCK_FU740	9
 
 extern struct __prci_clock __prci_init_clocks_fu740[NUM_CLOCK_FU740];
 
diff --git a/drivers/clk/sifive/sifive-prci.c b/drivers/clk/sifive/sifive-prci.c
index 1490b01ce629..9997a3fa4a38 100644
--- a/drivers/clk/sifive/sifive-prci.c
+++ b/drivers/clk/sifive/sifive-prci.c
@@ -453,6 +453,47 @@ void sifive_prci_hfpclkpllsel_use_hfpclkpll(struct __prci_data *pd)
 	r = __prci_readl(pd, PRCI_HFPCLKPLLSEL_OFFSET);	/* barrier */
 }
 
+/* PCIE AUX clock APIs for enable, disable. */
+int sifive_prci_pcie_aux_clock_is_enabled(struct clk_hw *hw)
+{
+	struct __prci_clock *pc = clk_hw_to_prci_clock(hw);
+	struct __prci_data *pd = pc->pd;
+	u32 r;
+
+	r = __prci_readl(pd, PRCI_PCIE_AUX_OFFSET);
+
+	if (r & PRCI_PCIE_AUX_EN_MASK)
+		return 1;
+	else
+		return 0;
+}
+
+int sifive_prci_pcie_aux_clock_enable(struct clk_hw *hw)
+{
+	struct __prci_clock *pc = clk_hw_to_prci_clock(hw);
+	struct __prci_data *pd = pc->pd;
+	u32 r __maybe_unused;
+
+	if (sifive_prci_pcie_aux_clock_is_enabled(hw))
+		return 0;
+
+	__prci_writel(1, PRCI_PCIE_AUX_OFFSET, pd);
+	r = __prci_readl(pd, PRCI_PCIE_AUX_OFFSET);	/* barrier */
+
+	return 0;
+}
+
+void sifive_prci_pcie_aux_clock_disable(struct clk_hw *hw)
+{
+	struct __prci_clock *pc = clk_hw_to_prci_clock(hw);
+	struct __prci_data *pd = pc->pd;
+	u32 r __maybe_unused;
+
+	__prci_writel(0, PRCI_PCIE_AUX_OFFSET, pd);
+	r = __prci_readl(pd, PRCI_PCIE_AUX_OFFSET);	/* barrier */
+
+}
+
 /**
  * __prci_register_clocks() - register clock controls in the PRCI
  * @dev: Linux struct device
diff --git a/drivers/clk/sifive/sifive-prci.h b/drivers/clk/sifive/sifive-prci.h
index dbdbd1722688..022c67cf053c 100644
--- a/drivers/clk/sifive/sifive-prci.h
+++ b/drivers/clk/sifive/sifive-prci.h
@@ -67,6 +67,11 @@
 #define PRCI_DDRPLLCFG1_CKE_SHIFT	31
 #define PRCI_DDRPLLCFG1_CKE_MASK	(0x1 << PRCI_DDRPLLCFG1_CKE_SHIFT)
 
+/* PCIEAUX */
+#define PRCI_PCIE_AUX_OFFSET		0x14
+#define PRCI_PCIE_AUX_EN_SHIFT		0
+#define PRCI_PCIE_AUX_EN_MASK		(0x1 << PRCI_PCIE_AUX_EN_SHIFT)
+
 /* GEMGXLPLLCFG0 */
 #define PRCI_GEMGXLPLLCFG0_OFFSET	0x1c
 #define PRCI_GEMGXLPLLCFG0_DIVR_SHIFT	0
@@ -296,4 +301,8 @@ unsigned long sifive_prci_tlclksel_recalc_rate(struct clk_hw *hw,
 unsigned long sifive_prci_hfpclkplldiv_recalc_rate(struct clk_hw *hw,
 						   unsigned long parent_rate);
 
+int sifive_prci_pcie_aux_clock_is_enabled(struct clk_hw *hw);
+int sifive_prci_pcie_aux_clock_enable(struct clk_hw *hw);
+void sifive_prci_pcie_aux_clock_disable(struct clk_hw *hw);
+
 #endif /* __SIFIVE_CLK_SIFIVE_PRCI_H */
diff --git a/include/dt-bindings/clock/sifive-fu740-prci.h b/include/dt-bindings/clock/sifive-fu740-prci.h
index cd7706ea5677..7899b7fee7db 100644
--- a/include/dt-bindings/clock/sifive-fu740-prci.h
+++ b/include/dt-bindings/clock/sifive-fu740-prci.h
@@ -19,5 +19,6 @@
 #define PRCI_CLK_CLTXPLL	       5
 #define PRCI_CLK_TLCLK		       6
 #define PRCI_CLK_PCLK		       7
+#define PRCI_CLK_PCIE_AUX	       8
 
 #endif	/* __DT_BINDINGS_CLOCK_SIFIVE_FU740_PRCI_H */
-- 
2.31.1


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

* [PATCH v6 2/6] clk: sifive: Use reset-simple in prci driver for PCIe driver
  2021-05-04 10:59 [PATCH v6 0/6] Add SiFive FU740 PCIe host controller driver support Greentime Hu
  2021-05-04 10:59 ` [PATCH v6 1/6] clk: sifive: Add pcie_aux clock in prci driver for PCIe driver Greentime Hu
@ 2021-05-04 10:59 ` Greentime Hu
  2021-05-04 10:59 ` [PATCH v6 3/6] MAINTAINERS: Add maintainers for SiFive FU740 " Greentime Hu
                   ` (4 subsequent siblings)
  6 siblings, 0 replies; 17+ messages in thread
From: Greentime Hu @ 2021-05-04 10:59 UTC (permalink / raw)
  To: greentime.hu, paul.walmsley, hes, erik.danie, zong.li, bhelgaas,
	robh+dt, aou, mturquette, sboyd, lorenzo.pieralisi, p.zabel,
	alex.dewar90, khilman, hayashi.kunihiko, vidyas, jh80.chung,
	linux-pci, devicetree, linux-riscv, linux-kernel, linux-clk,
	helgaas

We use reset-simple in this patch so that pcie driver can use
devm_reset_control_get() to get this reset data structure and use
reset_control_deassert() to deassert pcie_power_up_rst_n.

Signed-off-by: Greentime Hu <greentime.hu@sifive.com>
Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>
Acked-by: Stephen Boyd <sboyd@kernel.org>
---
 drivers/clk/sifive/Kconfig       |  2 ++
 drivers/clk/sifive/sifive-prci.c | 13 +++++++++++++
 drivers/clk/sifive/sifive-prci.h |  4 ++++
 drivers/reset/Kconfig            |  1 +
 4 files changed, 20 insertions(+)

diff --git a/drivers/clk/sifive/Kconfig b/drivers/clk/sifive/Kconfig
index 1c14eb20c066..9132c3c4aa86 100644
--- a/drivers/clk/sifive/Kconfig
+++ b/drivers/clk/sifive/Kconfig
@@ -10,6 +10,8 @@ if CLK_SIFIVE
 
 config CLK_SIFIVE_PRCI
 	bool "PRCI driver for SiFive SoCs"
+	select RESET_CONTROLLER
+	select RESET_SIMPLE
 	select CLK_ANALOGBITS_WRPLL_CLN28HPC
 	help
 	  Supports the Power Reset Clock interface (PRCI) IP block found in
diff --git a/drivers/clk/sifive/sifive-prci.c b/drivers/clk/sifive/sifive-prci.c
index 9997a3fa4a38..0d79ba31a793 100644
--- a/drivers/clk/sifive/sifive-prci.c
+++ b/drivers/clk/sifive/sifive-prci.c
@@ -588,6 +588,19 @@ static int sifive_prci_probe(struct platform_device *pdev)
 	if (IS_ERR(pd->va))
 		return PTR_ERR(pd->va);
 
+	pd->reset.rcdev.owner = THIS_MODULE;
+	pd->reset.rcdev.nr_resets = PRCI_RST_NR;
+	pd->reset.rcdev.ops = &reset_simple_ops;
+	pd->reset.rcdev.of_node = pdev->dev.of_node;
+	pd->reset.active_low = true;
+	pd->reset.membase = pd->va + PRCI_DEVICESRESETREG_OFFSET;
+	spin_lock_init(&pd->reset.lock);
+
+	r = devm_reset_controller_register(&pdev->dev, &pd->reset.rcdev);
+	if (r) {
+		dev_err(dev, "could not register reset controller: %d\n", r);
+		return r;
+	}
 	r = __prci_register_clocks(dev, pd, desc);
 	if (r) {
 		dev_err(dev, "could not register clocks: %d\n", r);
diff --git a/drivers/clk/sifive/sifive-prci.h b/drivers/clk/sifive/sifive-prci.h
index 022c67cf053c..91658a88af4e 100644
--- a/drivers/clk/sifive/sifive-prci.h
+++ b/drivers/clk/sifive/sifive-prci.h
@@ -11,6 +11,7 @@
 
 #include <linux/clk/analogbits-wrpll-cln28hpc.h>
 #include <linux/clk-provider.h>
+#include <linux/reset/reset-simple.h>
 #include <linux/platform_device.h>
 
 /*
@@ -121,6 +122,8 @@
 #define PRCI_DEVICESRESETREG_CHIPLINK_RST_N_MASK			\
 		(0x1 << PRCI_DEVICESRESETREG_CHIPLINK_RST_N_SHIFT)
 
+#define PRCI_RST_NR						7
+
 /* CLKMUXSTATUSREG */
 #define PRCI_CLKMUXSTATUSREG_OFFSET				0x2c
 #define PRCI_CLKMUXSTATUSREG_TLCLKSEL_STATUS_SHIFT		1
@@ -221,6 +224,7 @@
  */
 struct __prci_data {
 	void __iomem *va;
+	struct reset_simple_data reset;
 	struct clk_hw_onecell_data hw_clks;
 };
 
diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
index 4171c6f76385..0f40dadf5705 100644
--- a/drivers/reset/Kconfig
+++ b/drivers/reset/Kconfig
@@ -197,6 +197,7 @@ config RESET_SIMPLE
 	   - RCC reset controller in STM32 MCUs
 	   - Allwinner SoCs
 	   - ZTE's zx2967 family
+	   - SiFive FU740 SoCs
 
 config RESET_STM32MP157
 	bool "STM32MP157 Reset Driver" if COMPILE_TEST
-- 
2.31.1


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

* [PATCH v6 3/6] MAINTAINERS: Add maintainers for SiFive FU740 PCIe driver
  2021-05-04 10:59 [PATCH v6 0/6] Add SiFive FU740 PCIe host controller driver support Greentime Hu
  2021-05-04 10:59 ` [PATCH v6 1/6] clk: sifive: Add pcie_aux clock in prci driver for PCIe driver Greentime Hu
  2021-05-04 10:59 ` [PATCH v6 2/6] clk: sifive: Use reset-simple " Greentime Hu
@ 2021-05-04 10:59 ` Greentime Hu
  2021-05-04 10:59 ` [PATCH v6 4/6] dt-bindings: PCI: Add SiFive FU740 PCIe host controller Greentime Hu
                   ` (3 subsequent siblings)
  6 siblings, 0 replies; 17+ messages in thread
From: Greentime Hu @ 2021-05-04 10:59 UTC (permalink / raw)
  To: greentime.hu, paul.walmsley, hes, erik.danie, zong.li, bhelgaas,
	robh+dt, aou, mturquette, sboyd, lorenzo.pieralisi, p.zabel,
	alex.dewar90, khilman, hayashi.kunihiko, vidyas, jh80.chung,
	linux-pci, devicetree, linux-riscv, linux-kernel, linux-clk,
	helgaas

Here add maintainer information for SiFive FU740 PCIe driver.

Signed-off-by: Greentime Hu <greentime.hu@sifive.com>
---
 MAINTAINERS | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 9450e052f1b1..295711c81bef 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -13699,6 +13699,14 @@ S:	Maintained
 F:	Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
 F:	drivers/pci/controller/dwc/*imx6*
 
+PCI DRIVER FOR FU740
+M:	Paul Walmsley <paul.walmsley@sifive.com>
+M:	Greentime Hu <greentime.hu@sifive.com>
+L:	linux-pci@vger.kernel.org
+S:	Maintained
+F:	Documentation/devicetree/bindings/pci/sifive,fu740-pcie.yaml
+F:	drivers/pci/controller/dwc/pcie-fu740.c
+
 PCI DRIVER FOR INTEL VOLUME MANAGEMENT DEVICE (VMD)
 M:	Jonathan Derrick <jonathan.derrick@intel.com>
 L:	linux-pci@vger.kernel.org
-- 
2.31.1


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

* [PATCH v6 4/6] dt-bindings: PCI: Add SiFive FU740 PCIe host controller
  2021-05-04 10:59 [PATCH v6 0/6] Add SiFive FU740 PCIe host controller driver support Greentime Hu
                   ` (2 preceding siblings ...)
  2021-05-04 10:59 ` [PATCH v6 3/6] MAINTAINERS: Add maintainers for SiFive FU740 " Greentime Hu
@ 2021-05-04 10:59 ` Greentime Hu
  2021-05-04 10:59 ` [PATCH v6 5/6] PCI: fu740: Add SiFive FU740 PCIe host controller driver Greentime Hu
                   ` (2 subsequent siblings)
  6 siblings, 0 replies; 17+ messages in thread
From: Greentime Hu @ 2021-05-04 10:59 UTC (permalink / raw)
  To: greentime.hu, paul.walmsley, hes, erik.danie, zong.li, bhelgaas,
	robh+dt, aou, mturquette, sboyd, lorenzo.pieralisi, p.zabel,
	alex.dewar90, khilman, hayashi.kunihiko, vidyas, jh80.chung,
	linux-pci, devicetree, linux-riscv, linux-kernel, linux-clk,
	helgaas
  Cc: Rob Herring

Add PCIe host controller DT bindings of SiFive FU740.

Signed-off-by: Greentime Hu <greentime.hu@sifive.com>
Reviewed-by: Rob Herring <robh@kernel.org>
---
 .../bindings/pci/sifive,fu740-pcie.yaml       | 113 ++++++++++++++++++
 1 file changed, 113 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/sifive,fu740-pcie.yaml

diff --git a/Documentation/devicetree/bindings/pci/sifive,fu740-pcie.yaml b/Documentation/devicetree/bindings/pci/sifive,fu740-pcie.yaml
new file mode 100644
index 000000000000..b03cbb9b6602
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/sifive,fu740-pcie.yaml
@@ -0,0 +1,113 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pci/sifive,fu740-pcie.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: SiFive FU740 PCIe host controller
+
+description: |+
+  SiFive FU740 PCIe host controller is based on the Synopsys DesignWare
+  PCI core. It shares common features with the PCIe DesignWare core and
+  inherits common properties defined in
+  Documentation/devicetree/bindings/pci/designware-pcie.txt.
+
+maintainers:
+  - Paul Walmsley <paul.walmsley@sifive.com>
+  - Greentime Hu <greentime.hu@sifive.com>
+
+allOf:
+  - $ref: /schemas/pci/pci-bus.yaml#
+
+properties:
+  compatible:
+    const: sifive,fu740-pcie
+
+  reg:
+    maxItems: 3
+
+  reg-names:
+    items:
+      - const: dbi
+      - const: config
+      - const: mgmt
+
+  num-lanes:
+    const: 8
+
+  msi-parent: true
+
+  interrupt-names:
+    items:
+      - const: msi
+      - const: inta
+      - const: intb
+      - const: intc
+      - const: intd
+
+  resets:
+    description: A phandle to the PCIe power up reset line.
+    maxItems: 1
+
+  pwren-gpios:
+    description: Should specify the GPIO for controlling the PCI bus device power on.
+    maxItems: 1
+
+  reset-gpios:
+    maxItems: 1
+
+required:
+  - dma-coherent
+  - num-lanes
+  - interrupts
+  - interrupt-names
+  - interrupt-parent
+  - interrupt-map-mask
+  - interrupt-map
+  - clock-names
+  - clocks
+  - resets
+  - pwren-gpios
+  - reset-gpios
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    bus {
+        #address-cells = <2>;
+        #size-cells = <2>;
+        #include <dt-bindings/clock/sifive-fu740-prci.h>
+
+        pcie@e00000000 {
+            compatible = "sifive,fu740-pcie";
+            #address-cells = <3>;
+            #size-cells = <2>;
+            #interrupt-cells = <1>;
+            reg = <0xe 0x00000000 0x0 0x80000000>,
+                  <0xd 0xf0000000 0x0 0x10000000>,
+                  <0x0 0x100d0000 0x0 0x1000>;
+            reg-names = "dbi", "config", "mgmt";
+            device_type = "pci";
+            dma-coherent;
+            bus-range = <0x0 0xff>;
+            ranges = <0x81000000  0x0 0x60080000  0x0 0x60080000 0x0 0x10000>,      /* I/O */
+                     <0x82000000  0x0 0x60090000  0x0 0x60090000 0x0 0xff70000>,    /* mem */
+                     <0x82000000  0x0 0x70000000  0x0 0x70000000 0x0 0x1000000>,    /* mem */
+                     <0xc3000000 0x20 0x00000000 0x20 0x00000000 0x20 0x00000000>;  /* mem prefetchable */
+            num-lanes = <0x8>;
+            interrupts = <56>, <57>, <58>, <59>, <60>, <61>, <62>, <63>, <64>;
+            interrupt-names = "msi", "inta", "intb", "intc", "intd";
+            interrupt-parent = <&plic0>;
+            interrupt-map-mask = <0x0 0x0 0x0 0x7>;
+            interrupt-map = <0x0 0x0 0x0 0x1 &plic0 57>,
+                            <0x0 0x0 0x0 0x2 &plic0 58>,
+                            <0x0 0x0 0x0 0x3 &plic0 59>,
+                            <0x0 0x0 0x0 0x4 &plic0 60>;
+            clock-names = "pcie_aux";
+            clocks = <&prci PRCI_CLK_PCIE_AUX>;
+            resets = <&prci 4>;
+            pwren-gpios = <&gpio 5 0>;
+            reset-gpios = <&gpio 8 0>;
+        };
+    };
-- 
2.31.1


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

* [PATCH v6 5/6] PCI: fu740: Add SiFive FU740 PCIe host controller driver
  2021-05-04 10:59 [PATCH v6 0/6] Add SiFive FU740 PCIe host controller driver support Greentime Hu
                   ` (3 preceding siblings ...)
  2021-05-04 10:59 ` [PATCH v6 4/6] dt-bindings: PCI: Add SiFive FU740 PCIe host controller Greentime Hu
@ 2021-05-04 10:59 ` Greentime Hu
  2021-05-04 13:46   ` Bjorn Helgaas
  2021-05-04 10:59 ` [PATCH v6 6/6] riscv: dts: Add PCIe support for the SiFive FU740-C000 SoC Greentime Hu
  2021-05-04 11:28 ` [PATCH v6 0/6] Add SiFive FU740 PCIe host controller driver support Lorenzo Pieralisi
  6 siblings, 1 reply; 17+ messages in thread
From: Greentime Hu @ 2021-05-04 10:59 UTC (permalink / raw)
  To: greentime.hu, paul.walmsley, hes, erik.danie, zong.li, bhelgaas,
	robh+dt, aou, mturquette, sboyd, lorenzo.pieralisi, p.zabel,
	alex.dewar90, khilman, hayashi.kunihiko, vidyas, jh80.chung,
	linux-pci, devicetree, linux-riscv, linux-kernel, linux-clk,
	helgaas

From: Paul Walmsley <paul.walmsley@sifive.com>

Add driver for the SiFive FU740 PCIe host controller.
This controller is based on the DesignWare PCIe core.

Signed-off-by: Paul Walmsley <paul.walmsley@sifive.com>
Co-developed-by: Henry Styles <hes@sifive.com>
Signed-off-by: Henry Styles <hes@sifive.com>
Co-developed-by: Erik Danie <erik.danie@sifive.com>
Signed-off-by: Erik Danie <erik.danie@sifive.com>
Co-developed-by: Greentime Hu <greentime.hu@sifive.com>
Signed-off-by: Greentime Hu <greentime.hu@sifive.com>
---
 drivers/pci/controller/dwc/Kconfig      |  10 +
 drivers/pci/controller/dwc/Makefile     |   1 +
 drivers/pci/controller/dwc/pcie-fu740.c | 309 ++++++++++++++++++++++++
 3 files changed, 320 insertions(+)
 create mode 100644 drivers/pci/controller/dwc/pcie-fu740.c

diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
index 22c5529e9a65..255d43b1661b 100644
--- a/drivers/pci/controller/dwc/Kconfig
+++ b/drivers/pci/controller/dwc/Kconfig
@@ -318,4 +318,14 @@ 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_FU740
+	bool "SiFive FU740 PCIe host controller"
+	depends on PCI_MSI_IRQ_DOMAIN
+	depends on SOC_SIFIVE || COMPILE_TEST
+	depends on GPIOLIB
+	select PCIE_DW_HOST
+	help
+	  Say Y here if you want PCIe controller support for the SiFive
+	  FU740.
+
 endmenu
diff --git a/drivers/pci/controller/dwc/Makefile b/drivers/pci/controller/dwc/Makefile
index a751553fa0db..625f6aaeb5b8 100644
--- a/drivers/pci/controller/dwc/Makefile
+++ b/drivers/pci/controller/dwc/Makefile
@@ -5,6 +5,7 @@ obj-$(CONFIG_PCIE_DW_EP) += pcie-designware-ep.o
 obj-$(CONFIG_PCIE_DW_PLAT) += pcie-designware-plat.o
 obj-$(CONFIG_PCI_DRA7XX) += pci-dra7xx.o
 obj-$(CONFIG_PCI_EXYNOS) += pci-exynos.o
+obj-$(CONFIG_PCIE_FU740) += pcie-fu740.o
 obj-$(CONFIG_PCI_IMX6) += pci-imx6.o
 obj-$(CONFIG_PCIE_SPEAR13XX) += pcie-spear13xx.o
 obj-$(CONFIG_PCI_KEYSTONE) += pci-keystone.o
diff --git a/drivers/pci/controller/dwc/pcie-fu740.c b/drivers/pci/controller/dwc/pcie-fu740.c
new file mode 100644
index 000000000000..00cde9a248b5
--- /dev/null
+++ b/drivers/pci/controller/dwc/pcie-fu740.c
@@ -0,0 +1,309 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * FU740 DesignWare PCIe Controller integration
+ * Copyright (C) 2019-2021 SiFive, Inc.
+ * Paul Walmsley
+ * Greentime Hu
+ *
+ * Based in part on the i.MX6 PCIe host controller shim which is:
+ *
+ * Copyright (C) 2013 Kosagi
+ *		https://www.kosagi.com
+ */
+
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/gpio.h>
+#include <linux/gpio/consumer.h>
+#include <linux/kernel.h>
+#include <linux/mfd/syscon.h>
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/platform_device.h>
+#include <linux/regulator/consumer.h>
+#include <linux/resource.h>
+#include <linux/types.h>
+#include <linux/interrupt.h>
+#include <linux/iopoll.h>
+#include <linux/reset.h>
+
+#include "pcie-designware.h"
+
+#define to_fu740_pcie(x)	dev_get_drvdata((x)->dev)
+
+struct fu740_pcie {
+	struct dw_pcie pci;
+	void __iomem *mgmt_base;
+	struct gpio_desc *reset;
+	struct gpio_desc *pwren;
+	struct clk *pcie_aux;
+	struct reset_control *rst;
+};
+
+#define SIFIVE_DEVICESRESETREG		0x28
+
+#define PCIEX8MGMT_PERST_N		0x0
+#define PCIEX8MGMT_APP_LTSSM_ENABLE	0x10
+#define PCIEX8MGMT_APP_HOLD_PHY_RST	0x18
+#define PCIEX8MGMT_DEVICE_TYPE		0x708
+#define PCIEX8MGMT_PHY0_CR_PARA_ADDR	0x860
+#define PCIEX8MGMT_PHY0_CR_PARA_RD_EN	0x870
+#define PCIEX8MGMT_PHY0_CR_PARA_RD_DATA	0x878
+#define PCIEX8MGMT_PHY0_CR_PARA_SEL	0x880
+#define PCIEX8MGMT_PHY0_CR_PARA_WR_DATA	0x888
+#define PCIEX8MGMT_PHY0_CR_PARA_WR_EN	0x890
+#define PCIEX8MGMT_PHY0_CR_PARA_ACK	0x898
+#define PCIEX8MGMT_PHY1_CR_PARA_ADDR	0x8a0
+#define PCIEX8MGMT_PHY1_CR_PARA_RD_EN	0x8b0
+#define PCIEX8MGMT_PHY1_CR_PARA_RD_DATA	0x8b8
+#define PCIEX8MGMT_PHY1_CR_PARA_SEL	0x8c0
+#define PCIEX8MGMT_PHY1_CR_PARA_WR_DATA	0x8c8
+#define PCIEX8MGMT_PHY1_CR_PARA_WR_EN	0x8d0
+#define PCIEX8MGMT_PHY1_CR_PARA_ACK	0x8d8
+
+#define PCIEX8MGMT_PHY_CDR_TRACK_EN	BIT(0)
+#define PCIEX8MGMT_PHY_LOS_THRSHLD	BIT(5)
+#define PCIEX8MGMT_PHY_TERM_EN		BIT(9)
+#define PCIEX8MGMT_PHY_TERM_ACDC	BIT(10)
+#define PCIEX8MGMT_PHY_EN		BIT(11)
+#define PCIEX8MGMT_PHY_INIT_VAL		(PCIEX8MGMT_PHY_CDR_TRACK_EN|\
+					 PCIEX8MGMT_PHY_LOS_THRSHLD|\
+					 PCIEX8MGMT_PHY_TERM_EN|\
+					 PCIEX8MGMT_PHY_TERM_ACDC|\
+					 PCIEX8MGMT_PHY_EN)
+
+#define PCIEX8MGMT_PHY_LANEN_DIG_ASIC_RX_OVRD_IN_3	0x1008
+#define PCIEX8MGMT_PHY_LANE_OFF		0x100
+#define PCIEX8MGMT_PHY_LANE0_BASE	(PCIEX8MGMT_PHY_LANEN_DIG_ASIC_RX_OVRD_IN_3 + 0x100 * 0)
+#define PCIEX8MGMT_PHY_LANE1_BASE	(PCIEX8MGMT_PHY_LANEN_DIG_ASIC_RX_OVRD_IN_3 + 0x100 * 1)
+#define PCIEX8MGMT_PHY_LANE2_BASE	(PCIEX8MGMT_PHY_LANEN_DIG_ASIC_RX_OVRD_IN_3 + 0x100 * 2)
+#define PCIEX8MGMT_PHY_LANE3_BASE	(PCIEX8MGMT_PHY_LANEN_DIG_ASIC_RX_OVRD_IN_3 + 0x100 * 3)
+
+static void fu740_pcie_assert_reset(struct fu740_pcie *afp)
+{
+	/* Assert PERST_N GPIO */
+	gpiod_set_value_cansleep(afp->reset, 0);
+	/* Assert controller PERST_N */
+	writel_relaxed(0x0, afp->mgmt_base + PCIEX8MGMT_PERST_N);
+}
+
+static void fu740_pcie_deassert_reset(struct fu740_pcie *afp)
+{
+	/* Deassert controller PERST_N */
+	writel_relaxed(0x1, afp->mgmt_base + PCIEX8MGMT_PERST_N);
+	/* Deassert PERST_N GPIO */
+	gpiod_set_value_cansleep(afp->reset, 1);
+}
+
+static void fu740_pcie_power_on(struct fu740_pcie *afp)
+{
+	gpiod_set_value_cansleep(afp->pwren, 1);
+	/*
+	 * Ensure that PERST has been asserted for at least 100 ms.
+	 * Section 2.2 of PCI Express Card Electromechanical Specification
+	 * Revision 3.0
+	 */
+	msleep(100);
+}
+
+static void fu740_pcie_drive_reset(struct fu740_pcie *afp)
+{
+	fu740_pcie_assert_reset(afp);
+	fu740_pcie_power_on(afp);
+	fu740_pcie_deassert_reset(afp);
+}
+
+static void fu740_phyregwrite(const uint8_t phy, const uint16_t addr,
+			      const uint16_t wrdata, struct fu740_pcie *afp)
+{
+	struct device *dev = afp->pci.dev;
+	void __iomem *phy_cr_para_addr;
+	void __iomem *phy_cr_para_wr_data;
+	void __iomem *phy_cr_para_wr_en;
+	void __iomem *phy_cr_para_ack;
+	int ret, val;
+
+	/* Setup */
+	if (phy) {
+		phy_cr_para_addr = afp->mgmt_base + PCIEX8MGMT_PHY1_CR_PARA_ADDR;
+		phy_cr_para_wr_data = afp->mgmt_base + PCIEX8MGMT_PHY1_CR_PARA_WR_DATA;
+		phy_cr_para_wr_en = afp->mgmt_base + PCIEX8MGMT_PHY1_CR_PARA_WR_EN;
+		phy_cr_para_ack = afp->mgmt_base + PCIEX8MGMT_PHY1_CR_PARA_ACK;
+	} else {
+		phy_cr_para_addr = afp->mgmt_base + PCIEX8MGMT_PHY0_CR_PARA_ADDR;
+		phy_cr_para_wr_data = afp->mgmt_base + PCIEX8MGMT_PHY0_CR_PARA_WR_DATA;
+		phy_cr_para_wr_en = afp->mgmt_base + PCIEX8MGMT_PHY0_CR_PARA_WR_EN;
+		phy_cr_para_ack = afp->mgmt_base + PCIEX8MGMT_PHY0_CR_PARA_ACK;
+	}
+
+	writel_relaxed(addr, phy_cr_para_addr);
+	writel_relaxed(wrdata, phy_cr_para_wr_data);
+	writel_relaxed(1, phy_cr_para_wr_en);
+
+	/* Wait for wait_idle */
+	ret = readl_poll_timeout(phy_cr_para_ack, val, val, 10, 5000);
+	if (ret)
+		dev_warn(dev, "Wait for wait_idle state failed!\n");
+
+	/* Clear */
+	writel_relaxed(0, phy_cr_para_wr_en);
+
+	/* Wait for ~wait_idle */
+	ret = readl_poll_timeout(phy_cr_para_ack, val, !val, 10, 5000);
+	if (ret)
+		dev_warn(dev, "Wait for !wait_idle state failed!\n");
+}
+
+static void fu740_pcie_init_phy(struct fu740_pcie *afp)
+{
+	/* Enable phy cr_para_sel interfaces */
+	writel_relaxed(0x1, afp->mgmt_base + PCIEX8MGMT_PHY0_CR_PARA_SEL);
+	writel_relaxed(0x1, afp->mgmt_base + PCIEX8MGMT_PHY1_CR_PARA_SEL);
+
+	/*
+	 * Wait 10 cr_para cycles to guarantee that the registers are ready
+	 * to be edited.
+	 */
+	ndelay(10);
+
+	/* Set PHY AC termination mode */
+	fu740_phyregwrite(0, PCIEX8MGMT_PHY_LANE0_BASE, PCIEX8MGMT_PHY_INIT_VAL, afp);
+	fu740_phyregwrite(0, PCIEX8MGMT_PHY_LANE1_BASE, PCIEX8MGMT_PHY_INIT_VAL, afp);
+	fu740_phyregwrite(0, PCIEX8MGMT_PHY_LANE2_BASE, PCIEX8MGMT_PHY_INIT_VAL, afp);
+	fu740_phyregwrite(0, PCIEX8MGMT_PHY_LANE3_BASE, PCIEX8MGMT_PHY_INIT_VAL, afp);
+	fu740_phyregwrite(1, PCIEX8MGMT_PHY_LANE0_BASE, PCIEX8MGMT_PHY_INIT_VAL, afp);
+	fu740_phyregwrite(1, PCIEX8MGMT_PHY_LANE1_BASE, PCIEX8MGMT_PHY_INIT_VAL, afp);
+	fu740_phyregwrite(1, PCIEX8MGMT_PHY_LANE2_BASE, PCIEX8MGMT_PHY_INIT_VAL, afp);
+	fu740_phyregwrite(1, PCIEX8MGMT_PHY_LANE3_BASE, PCIEX8MGMT_PHY_INIT_VAL, afp);
+}
+
+static int fu740_pcie_start_link(struct dw_pcie *pci)
+{
+	struct device *dev = pci->dev;
+	struct fu740_pcie *afp = dev_get_drvdata(dev);
+
+	/* Enable LTSSM */
+	writel_relaxed(0x1, afp->mgmt_base + PCIEX8MGMT_APP_LTSSM_ENABLE);
+	return 0;
+}
+
+static int fu740_pcie_host_init(struct pcie_port *pp)
+{
+	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
+	struct fu740_pcie *afp = to_fu740_pcie(pci);
+	struct device *dev = pci->dev;
+	int ret;
+
+	/* Power on reset */
+	fu740_pcie_drive_reset(afp);
+
+	/* Enable pcieauxclk */
+	ret = clk_prepare_enable(afp->pcie_aux);
+	if (ret) {
+		dev_err(dev, "unable to enable pcie_aux clock\n");
+		return ret;
+	}
+
+	/*
+	 * Assert hold_phy_rst (hold the controller LTSSM in reset after
+	 * power_up_rst_n for register programming with cr_para)
+	 */
+	writel_relaxed(0x1, afp->mgmt_base + PCIEX8MGMT_APP_HOLD_PHY_RST);
+
+	/* Deassert power_up_rst_n */
+	ret = reset_control_deassert(afp->rst);
+	if (ret) {
+		dev_err(dev, "unable to deassert pcie_power_up_rst_n\n");
+		return ret;
+	}
+
+	fu740_pcie_init_phy(afp);
+
+	/* Disable pcieauxclk */
+	clk_disable_unprepare(afp->pcie_aux);
+	/* Clear hold_phy_rst */
+	writel_relaxed(0x0, afp->mgmt_base + PCIEX8MGMT_APP_HOLD_PHY_RST);
+	/* Enable pcieauxclk */
+	ret = clk_prepare_enable(afp->pcie_aux);
+	/* Set RC mode */
+	writel_relaxed(0x4, afp->mgmt_base + PCIEX8MGMT_DEVICE_TYPE);
+
+	return 0;
+}
+
+static const struct dw_pcie_host_ops fu740_pcie_host_ops = {
+	.host_init = fu740_pcie_host_init,
+};
+
+static const struct dw_pcie_ops dw_pcie_ops = {
+	.start_link = fu740_pcie_start_link,
+};
+
+static int fu740_pcie_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct dw_pcie *pci;
+	struct fu740_pcie *afp;
+
+	afp = devm_kzalloc(dev, sizeof(*afp), GFP_KERNEL);
+	if (!afp)
+		return -ENOMEM;
+	pci = &afp->pci;
+	pci->dev = dev;
+	pci->ops = &dw_pcie_ops;
+	pci->pp.ops = &fu740_pcie_host_ops;
+
+	/* SiFive specific region: mgmt */
+	afp->mgmt_base = devm_platform_ioremap_resource_byname(pdev, "mgmt");
+	if (IS_ERR(afp->mgmt_base))
+		return PTR_ERR(afp->mgmt_base);
+
+	/* Fetch GPIOs */
+	afp->reset = devm_gpiod_get_optional(dev, "reset-gpios", GPIOD_OUT_LOW);
+	if (IS_ERR(afp->reset))
+		return dev_err_probe(dev, PTR_ERR(afp->reset), "unable to get reset-gpios\n");
+
+	afp->pwren = devm_gpiod_get_optional(dev, "pwren-gpios", GPIOD_OUT_LOW);
+	if (IS_ERR(afp->pwren))
+		return dev_err_probe(dev, PTR_ERR(afp->pwren), "unable to get pwren-gpios\n");
+
+	/* Fetch clocks */
+	afp->pcie_aux = devm_clk_get(dev, "pcie_aux");
+	if (IS_ERR(afp->pcie_aux))
+		return dev_err_probe(dev, PTR_ERR(afp->pcie_aux),
+					     "pcie_aux clock source missing or invalid\n");
+
+	/* Fetch reset */
+	afp->rst = devm_reset_control_get_exclusive(dev, NULL);
+	if (IS_ERR(afp->rst))
+		return dev_err_probe(dev, PTR_ERR(afp->rst), "unable to get reset\n");
+
+	platform_set_drvdata(pdev, afp);
+
+	return dw_pcie_host_init(&pci->pp);
+}
+
+static void fu740_pcie_shutdown(struct platform_device *pdev)
+{
+	struct fu740_pcie *afp = platform_get_drvdata(pdev);
+
+	/* Bring down link, so bootloader gets clean state in case of reboot */
+	fu740_pcie_assert_reset(afp);
+}
+
+static const struct of_device_id fu740_pcie_of_match[] = {
+	{ .compatible = "sifive,fu740-pcie", },
+	{},
+};
+
+static struct platform_driver fu740_pcie_driver = {
+	.driver = {
+		   .name = "fu740-pcie",
+		   .of_match_table = fu740_pcie_of_match,
+		   .suppress_bind_attrs = true,
+	},
+	.probe = fu740_pcie_probe,
+	.shutdown = fu740_pcie_shutdown,
+};
+
+builtin_platform_driver(fu740_pcie_driver);
-- 
2.31.1


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

* [PATCH v6 6/6] riscv: dts: Add PCIe support for the SiFive FU740-C000 SoC
  2021-05-04 10:59 [PATCH v6 0/6] Add SiFive FU740 PCIe host controller driver support Greentime Hu
                   ` (4 preceding siblings ...)
  2021-05-04 10:59 ` [PATCH v6 5/6] PCI: fu740: Add SiFive FU740 PCIe host controller driver Greentime Hu
@ 2021-05-04 10:59 ` Greentime Hu
  2021-05-04 11:28 ` [PATCH v6 0/6] Add SiFive FU740 PCIe host controller driver support Lorenzo Pieralisi
  6 siblings, 0 replies; 17+ messages in thread
From: Greentime Hu @ 2021-05-04 10:59 UTC (permalink / raw)
  To: greentime.hu, paul.walmsley, hes, erik.danie, zong.li, bhelgaas,
	robh+dt, aou, mturquette, sboyd, lorenzo.pieralisi, p.zabel,
	alex.dewar90, khilman, hayashi.kunihiko, vidyas, jh80.chung,
	linux-pci, devicetree, linux-riscv, linux-kernel, linux-clk,
	helgaas
  Cc: Palmer Dabbelt

Signed-off-by: Greentime Hu <greentime.hu@sifive.com>
Acked-by: Palmer Dabbelt <palmerdabbelt@google.com>
---
 arch/riscv/boot/dts/sifive/fu740-c000.dtsi | 33 ++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/arch/riscv/boot/dts/sifive/fu740-c000.dtsi b/arch/riscv/boot/dts/sifive/fu740-c000.dtsi
index eeb4f8c3e0e7..8eef82e4199f 100644
--- a/arch/riscv/boot/dts/sifive/fu740-c000.dtsi
+++ b/arch/riscv/boot/dts/sifive/fu740-c000.dtsi
@@ -159,6 +159,7 @@ prci: clock-controller@10000000 {
 			reg = <0x0 0x10000000 0x0 0x1000>;
 			clocks = <&hfclk>, <&rtcclk>;
 			#clock-cells = <1>;
+			#reset-cells = <1>;
 		};
 		uart0: serial@10010000 {
 			compatible = "sifive,fu740-c000-uart", "sifive,uart0";
@@ -289,5 +290,37 @@ gpio: gpio@10060000 {
 			clocks = <&prci PRCI_CLK_PCLK>;
 			status = "disabled";
 		};
+		pcie@e00000000 {
+			compatible = "sifive,fu740-pcie";
+			#address-cells = <3>;
+			#size-cells = <2>;
+			#interrupt-cells = <1>;
+			reg = <0xe 0x00000000 0x0 0x80000000>,
+			      <0xd 0xf0000000 0x0 0x10000000>,
+			      <0x0 0x100d0000 0x0 0x1000>;
+			reg-names = "dbi", "config", "mgmt";
+			device_type = "pci";
+			dma-coherent;
+			bus-range = <0x0 0xff>;
+			ranges = <0x81000000  0x0 0x60080000  0x0 0x60080000 0x0 0x10000>,      /* I/O */
+				 <0x82000000  0x0 0x60090000  0x0 0x60090000 0x0 0xff70000>,    /* mem */
+				 <0x82000000  0x0 0x70000000  0x0 0x70000000 0x0 0x1000000>,    /* mem */
+				 <0xc3000000 0x20 0x00000000 0x20 0x00000000 0x20 0x00000000>;  /* mem prefetchable */
+			num-lanes = <0x8>;
+			interrupts = <56>, <57>, <58>, <59>, <60>, <61>, <62>, <63>, <64>;
+			interrupt-names = "msi", "inta", "intb", "intc", "intd";
+			interrupt-parent = <&plic0>;
+			interrupt-map-mask = <0x0 0x0 0x0 0x7>;
+			interrupt-map = <0x0 0x0 0x0 0x1 &plic0 57>,
+					<0x0 0x0 0x0 0x2 &plic0 58>,
+					<0x0 0x0 0x0 0x3 &plic0 59>,
+					<0x0 0x0 0x0 0x4 &plic0 60>;
+			clock-names = "pcie_aux";
+			clocks = <&prci PRCI_CLK_PCIE_AUX>;
+			pwren-gpios = <&gpio 5 0>;
+			reset-gpios = <&gpio 8 0>;
+			resets = <&prci 4>;
+			status = "okay";
+		};
 	};
 };
-- 
2.31.1


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

* Re: [PATCH v6 0/6] Add SiFive FU740 PCIe host controller driver support
  2021-05-04 10:59 [PATCH v6 0/6] Add SiFive FU740 PCIe host controller driver support Greentime Hu
                   ` (5 preceding siblings ...)
  2021-05-04 10:59 ` [PATCH v6 6/6] riscv: dts: Add PCIe support for the SiFive FU740-C000 SoC Greentime Hu
@ 2021-05-04 11:28 ` Lorenzo Pieralisi
  6 siblings, 0 replies; 17+ messages in thread
From: Lorenzo Pieralisi @ 2021-05-04 11:28 UTC (permalink / raw)
  To: linux-kernel, linux-riscv, paul.walmsley, devicetree, linux-pci,
	hayashi.kunihiko, sboyd, vidyas, aou, helgaas, linux-clk,
	bhelgaas, zong.li, hes, alex.dewar90, erik.danie, jh80.chung,
	Greentime Hu, mturquette, p.zabel, robh+dt, khilman
  Cc: Lorenzo Pieralisi

On Tue, 4 May 2021 18:59:34 +0800, Greentime Hu wrote:
> This patchset includes SiFive FU740 PCIe host controller driver. We also
> add pcie_aux clock and pcie_power_on_reset controller to prci driver for
> PCIe driver to use it.
> 
> This is tested with e1000e: Intel(R) PRO/1000 Network Card, AMD Radeon R5
> 230 graphics card and SP M.2 PCIe Gen 3 SSD in SiFive Unmatched based on
> v5.11 Linux kernel.
> 
> [...]

Applied to pci/risc-v, thanks.

[1/6] clk: sifive: Add pcie_aux clock in prci driver for PCIe driver
      https://git.kernel.org/lpieralisi/pci/c/c61287bf17
[2/6] clk: sifive: Use reset-simple in prci driver for PCIe driver
      https://git.kernel.org/lpieralisi/pci/c/e4d368e0b6
[3/6] MAINTAINERS: Add maintainers for SiFive FU740 PCIe driver
      https://git.kernel.org/lpieralisi/pci/c/2da0dd5e30
[4/6] dt-bindings: PCI: Add SiFive FU740 PCIe host controller
      https://git.kernel.org/lpieralisi/pci/c/43cea116be
[5/6] PCI: fu740: Add SiFive FU740 PCIe host controller driver
      https://git.kernel.org/lpieralisi/pci/c/d5f9eb3dbb
[6/6] riscv: dts: Add PCIe support for the SiFive FU740-C000 SoC
      https://git.kernel.org/lpieralisi/pci/c/dc69e229c1

Thanks,
Lorenzo

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

* Re: [PATCH v6 1/6] clk: sifive: Add pcie_aux clock in prci driver for PCIe driver
  2021-05-04 10:59 ` [PATCH v6 1/6] clk: sifive: Add pcie_aux clock in prci driver for PCIe driver Greentime Hu
@ 2021-05-04 12:24   ` Leon Romanovsky
  2021-05-04 16:23     ` Bjorn Helgaas
  0 siblings, 1 reply; 17+ messages in thread
From: Leon Romanovsky @ 2021-05-04 12:24 UTC (permalink / raw)
  To: Greentime Hu
  Cc: paul.walmsley, hes, erik.danie, zong.li, bhelgaas, robh+dt, aou,
	mturquette, sboyd, lorenzo.pieralisi, p.zabel, alex.dewar90,
	khilman, hayashi.kunihiko, vidyas, jh80.chung, linux-pci,
	devicetree, linux-riscv, linux-kernel, linux-clk, helgaas

On Tue, May 04, 2021 at 06:59:35PM +0800, Greentime Hu wrote:
> We add pcie_aux clock in this patch so that pcie driver can use
> clk_prepare_enable() and clk_disable_unprepare() to enable and disable
> pcie_aux clock.
> 
> Signed-off-by: Greentime Hu <greentime.hu@sifive.com>
> Acked-by: Stephen Boyd <sboyd@kernel.org>
> ---
>  drivers/clk/sifive/fu740-prci.c               | 11 +++++
>  drivers/clk/sifive/fu740-prci.h               |  2 +-
>  drivers/clk/sifive/sifive-prci.c              | 41 +++++++++++++++++++
>  drivers/clk/sifive/sifive-prci.h              |  9 ++++
>  include/dt-bindings/clock/sifive-fu740-prci.h |  1 +
>  5 files changed, 63 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/sifive/fu740-prci.c b/drivers/clk/sifive/fu740-prci.c
> index 764d1097aa51..53f6e00a03b9 100644
> --- a/drivers/clk/sifive/fu740-prci.c
> +++ b/drivers/clk/sifive/fu740-prci.c
> @@ -72,6 +72,12 @@ static const struct clk_ops sifive_fu740_prci_hfpclkplldiv_clk_ops = {
>  	.recalc_rate = sifive_prci_hfpclkplldiv_recalc_rate,

<...>

> +/* PCIE AUX clock APIs for enable, disable. */
> +int sifive_prci_pcie_aux_clock_is_enabled(struct clk_hw *hw)

It should be bool

> +{
> +	struct __prci_clock *pc = clk_hw_to_prci_clock(hw);
> +	struct __prci_data *pd = pc->pd;
> +	u32 r;
> +
> +	r = __prci_readl(pd, PRCI_PCIE_AUX_OFFSET);
> +
> +	if (r & PRCI_PCIE_AUX_EN_MASK)
> +		return 1;
> +	else
> +		return 0;
> +}

and here simple "return r & PRCI_PCIE_AUX_EN_MASK;"

> +
> +int sifive_prci_pcie_aux_clock_enable(struct clk_hw *hw)
> +{
> +	struct __prci_clock *pc = clk_hw_to_prci_clock(hw);
> +	struct __prci_data *pd = pc->pd;
> +	u32 r __maybe_unused;
> +
> +	if (sifive_prci_pcie_aux_clock_is_enabled(hw))
> +		return 0;

You actually call to this new function only once, put your
__prci_readl() here.

Thanks

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

* Re: [PATCH v6 5/6] PCI: fu740: Add SiFive FU740 PCIe host controller driver
  2021-05-04 10:59 ` [PATCH v6 5/6] PCI: fu740: Add SiFive FU740 PCIe host controller driver Greentime Hu
@ 2021-05-04 13:46   ` Bjorn Helgaas
  2021-05-04 14:23     ` Lorenzo Pieralisi
  2021-05-05  4:26     ` Greentime Hu
  0 siblings, 2 replies; 17+ messages in thread
From: Bjorn Helgaas @ 2021-05-04 13:46 UTC (permalink / raw)
  To: Greentime Hu
  Cc: paul.walmsley, hes, erik.danie, zong.li, bhelgaas, robh+dt, aou,
	mturquette, sboyd, lorenzo.pieralisi, p.zabel, alex.dewar90,
	khilman, hayashi.kunihiko, vidyas, jh80.chung, linux-pci,
	devicetree, linux-riscv, linux-kernel, linux-clk

On Tue, May 04, 2021 at 06:59:39PM +0800, Greentime Hu wrote:
> From: Paul Walmsley <paul.walmsley@sifive.com>
> 
> Add driver for the SiFive FU740 PCIe host controller.
> This controller is based on the DesignWare PCIe core.
> 
> Signed-off-by: Paul Walmsley <paul.walmsley@sifive.com>
> Co-developed-by: Henry Styles <hes@sifive.com>
> Signed-off-by: Henry Styles <hes@sifive.com>
> Co-developed-by: Erik Danie <erik.danie@sifive.com>
> Signed-off-by: Erik Danie <erik.danie@sifive.com>
> Co-developed-by: Greentime Hu <greentime.hu@sifive.com>
> Signed-off-by: Greentime Hu <greentime.hu@sifive.com>
> ---
>  drivers/pci/controller/dwc/Kconfig      |  10 +
>  drivers/pci/controller/dwc/Makefile     |   1 +
>  drivers/pci/controller/dwc/pcie-fu740.c | 309 ++++++++++++++++++++++++
>  3 files changed, 320 insertions(+)
>  create mode 100644 drivers/pci/controller/dwc/pcie-fu740.c
> 
> diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
> index 22c5529e9a65..255d43b1661b 100644
> --- a/drivers/pci/controller/dwc/Kconfig
> +++ b/drivers/pci/controller/dwc/Kconfig
> @@ -318,4 +318,14 @@ 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_FU740
> +	bool "SiFive FU740 PCIe host controller"
> +	depends on PCI_MSI_IRQ_DOMAIN
> +	depends on SOC_SIFIVE || COMPILE_TEST
> +	depends on GPIOLIB

1) I'm a little disappointed that I reported the build issue 6 days
   ago when we were already in the merge window, and it's taken until
   now to make some progress.

2) I would prefer not to depend on GPIOLIB because it reduces
   compile-test coverage.  For example, the x86_64 defconfig does not
   enable GPIOLIB, so one must manually enable it to even be able to
   enable PCIE_FU740.

   Many other PCI controller drivers use GPIO, but no others depend on
   GPIOLIB, so I infer that in the !GPIOLIB case, gpio/consumer.h
   provides the stubs required for compile testing.

   We could have a conversation about whether it's better to
   explicitly depend on GPIOLIB here, or whether building a working
   FU740 driver implicitly depends on GPIOLIB being selected
   elsewhere.  That implicit dependency *is* a little obscure, but I
   think that's what other drivers currently do, and I'd like to do
   this consistently unless there's a good reason otherwise.

   Here are some examples of other drivers:

   dwc/pci-dra7xx.c:
     config PCI_DRA7XX_HOST
       depends on SOC_DRA7XX || COMPILE_TEST

     config SOC_DRA7XX
       select ARCH_OMAP2PLUS

     config ARCH_OMAP2PLUS
       select GPIOLIB

   dwc/pci-meson.c:
     config PCI_MESON
       # doesn't, but probably *should* depend on "ARCH_MESON || COMPILE_TEST"

     menuconfig ARCH_MESON
       select GPIOLIB

   dwc/pcie-qcom.c:
     config PCIE_QCOM
       depends on OF && (ARCH_QCOM || COMPILE_TEST)

     config ARCH_QCOM
       select GPIOLIB

   pcie-rockchip.c:
     config PCIE_ROCKCHIP_HOST
       depends on ARCH_ROCKCHIP || COMPILE_TEST

     config ARCH_ROCKCHIP
       select GPIOLIB

> +	select PCIE_DW_HOST
> +	help
> +	  Say Y here if you want PCIe controller support for the SiFive
> +	  FU740.
> +
>  endmenu

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

* Re: [PATCH v6 5/6] PCI: fu740: Add SiFive FU740 PCIe host controller driver
  2021-05-04 13:46   ` Bjorn Helgaas
@ 2021-05-04 14:23     ` Lorenzo Pieralisi
  2021-05-05  4:26     ` Greentime Hu
  1 sibling, 0 replies; 17+ messages in thread
From: Lorenzo Pieralisi @ 2021-05-04 14:23 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Greentime Hu, paul.walmsley, hes, erik.danie, zong.li, bhelgaas,
	robh+dt, aou, mturquette, sboyd, p.zabel, alex.dewar90, khilman,
	hayashi.kunihiko, vidyas, jh80.chung, linux-pci, devicetree,
	linux-riscv, linux-kernel, linux-clk

On Tue, May 04, 2021 at 08:46:32AM -0500, Bjorn Helgaas wrote:
> On Tue, May 04, 2021 at 06:59:39PM +0800, Greentime Hu wrote:
> > From: Paul Walmsley <paul.walmsley@sifive.com>
> > 
> > Add driver for the SiFive FU740 PCIe host controller.
> > This controller is based on the DesignWare PCIe core.
> > 
> > Signed-off-by: Paul Walmsley <paul.walmsley@sifive.com>
> > Co-developed-by: Henry Styles <hes@sifive.com>
> > Signed-off-by: Henry Styles <hes@sifive.com>
> > Co-developed-by: Erik Danie <erik.danie@sifive.com>
> > Signed-off-by: Erik Danie <erik.danie@sifive.com>
> > Co-developed-by: Greentime Hu <greentime.hu@sifive.com>
> > Signed-off-by: Greentime Hu <greentime.hu@sifive.com>
> > ---
> >  drivers/pci/controller/dwc/Kconfig      |  10 +
> >  drivers/pci/controller/dwc/Makefile     |   1 +
> >  drivers/pci/controller/dwc/pcie-fu740.c | 309 ++++++++++++++++++++++++
> >  3 files changed, 320 insertions(+)
> >  create mode 100644 drivers/pci/controller/dwc/pcie-fu740.c
> > 
> > diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
> > index 22c5529e9a65..255d43b1661b 100644
> > --- a/drivers/pci/controller/dwc/Kconfig
> > +++ b/drivers/pci/controller/dwc/Kconfig
> > @@ -318,4 +318,14 @@ 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_FU740
> > +	bool "SiFive FU740 PCIe host controller"
> > +	depends on PCI_MSI_IRQ_DOMAIN
> > +	depends on SOC_SIFIVE || COMPILE_TEST
> > +	depends on GPIOLIB
> 
> 1) I'm a little disappointed that I reported the build issue 6 days
>    ago when we were already in the merge window, and it's taken until
>    now to make some progress.
> 
> 2) I would prefer not to depend on GPIOLIB because it reduces
>    compile-test coverage.  For example, the x86_64 defconfig does not
>    enable GPIOLIB, so one must manually enable it to even be able to
>    enable PCIE_FU740.
> 
>    Many other PCI controller drivers use GPIO, but no others depend on
>    GPIOLIB, so I infer that in the !GPIOLIB case, gpio/consumer.h
>    provides the stubs required for compile testing.
> 
>    We could have a conversation about whether it's better to
>    explicitly depend on GPIOLIB here, or whether building a working
>    FU740 driver implicitly depends on GPIOLIB being selected
>    elsewhere.  That implicit dependency *is* a little obscure, but I
>    think that's what other drivers currently do, and I'd like to do
>    this consistently unless there's a good reason otherwise.

I will drop the explicit GPIOLIB dependency and push it out. For (1) I
agree with you - I merged when I received some input - it is reasonable
to avoid adding it to v5.13 material for this reason, apologies.

Thanks,
Lorenzo

>    Here are some examples of other drivers:
> 
>    dwc/pci-dra7xx.c:
>      config PCI_DRA7XX_HOST
>        depends on SOC_DRA7XX || COMPILE_TEST
> 
>      config SOC_DRA7XX
>        select ARCH_OMAP2PLUS
> 
>      config ARCH_OMAP2PLUS
>        select GPIOLIB
> 
>    dwc/pci-meson.c:
>      config PCI_MESON
>        # doesn't, but probably *should* depend on "ARCH_MESON || COMPILE_TEST"
> 
>      menuconfig ARCH_MESON
>        select GPIOLIB
> 
>    dwc/pcie-qcom.c:
>      config PCIE_QCOM
>        depends on OF && (ARCH_QCOM || COMPILE_TEST)
> 
>      config ARCH_QCOM
>        select GPIOLIB
> 
>    pcie-rockchip.c:
>      config PCIE_ROCKCHIP_HOST
>        depends on ARCH_ROCKCHIP || COMPILE_TEST
> 
>      config ARCH_ROCKCHIP
>        select GPIOLIB
> 
> > +	select PCIE_DW_HOST
> > +	help
> > +	  Say Y here if you want PCIe controller support for the SiFive
> > +	  FU740.
> > +
> >  endmenu

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

* Re: [PATCH v6 1/6] clk: sifive: Add pcie_aux clock in prci driver for PCIe driver
  2021-05-04 12:24   ` Leon Romanovsky
@ 2021-05-04 16:23     ` Bjorn Helgaas
  2021-05-04 18:12       ` Leon Romanovsky
  0 siblings, 1 reply; 17+ messages in thread
From: Bjorn Helgaas @ 2021-05-04 16:23 UTC (permalink / raw)
  To: Leon Romanovsky
  Cc: Greentime Hu, paul.walmsley, hes, erik.danie, zong.li, bhelgaas,
	robh+dt, aou, mturquette, sboyd, lorenzo.pieralisi, p.zabel,
	alex.dewar90, khilman, hayashi.kunihiko, vidyas, jh80.chung,
	linux-pci, devicetree, linux-riscv, linux-kernel, linux-clk

On Tue, May 04, 2021 at 03:24:19PM +0300, Leon Romanovsky wrote:
> On Tue, May 04, 2021 at 06:59:35PM +0800, Greentime Hu wrote:
> > We add pcie_aux clock in this patch so that pcie driver can use
> > clk_prepare_enable() and clk_disable_unprepare() to enable and disable
> > pcie_aux clock.
> > 
> > Signed-off-by: Greentime Hu <greentime.hu@sifive.com>
> > Acked-by: Stephen Boyd <sboyd@kernel.org>
> > ---
> >  drivers/clk/sifive/fu740-prci.c               | 11 +++++
> >  drivers/clk/sifive/fu740-prci.h               |  2 +-
> >  drivers/clk/sifive/sifive-prci.c              | 41 +++++++++++++++++++
> >  drivers/clk/sifive/sifive-prci.h              |  9 ++++
> >  include/dt-bindings/clock/sifive-fu740-prci.h |  1 +
> >  5 files changed, 63 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/clk/sifive/fu740-prci.c b/drivers/clk/sifive/fu740-prci.c
> > index 764d1097aa51..53f6e00a03b9 100644
> > --- a/drivers/clk/sifive/fu740-prci.c
> > +++ b/drivers/clk/sifive/fu740-prci.c
> > @@ -72,6 +72,12 @@ static const struct clk_ops sifive_fu740_prci_hfpclkplldiv_clk_ops = {
> >  	.recalc_rate = sifive_prci_hfpclkplldiv_recalc_rate,
> 
> <...>
> 
> > +/* PCIE AUX clock APIs for enable, disable. */
> > +int sifive_prci_pcie_aux_clock_is_enabled(struct clk_hw *hw)
> 
> It should be bool

It's used via this function pointer:

  struct clk_ops {
    int             (*is_enabled)(struct clk_hw *hw);

so I think "int" is actually appropriate here.

There are some weird/interesting bool vs int usages nearby, though:

  "bool __is_clk_gate_enabled()" goes to some trouble to convert
  int to bool ("return (reg_val & bit_mask) != 0;"), and then
  kona_peri_clk_is_enabled() converts the bool back to int ("return
  is_clk_gate_enabled(bcm_clk->ccu, gate) ? 1 : 0;").

  "int lpc32xx_clk_gate_is_enabled()" actually returns a bool that is
  implicitly converted to int.

  Many *_is_enabled() functions return !!(...) where !! is an
  int-to-bool conversion that is arguably unnecessary and again
  results in an implicit conversion to int.

I don't see any *problems* with any of these; it just seems like a
little more mental effort to think about all the explicit and implicit
conversions going on.

> > +int sifive_prci_pcie_aux_clock_enable(struct clk_hw *hw)
> > +{
> > +	struct __prci_clock *pc = clk_hw_to_prci_clock(hw);
> > +	struct __prci_data *pd = pc->pd;
> > +	u32 r __maybe_unused;
> > +
> > +	if (sifive_prci_pcie_aux_clock_is_enabled(hw))
> > +		return 0;
> 
> You actually call to this new function only once, put your
> __prci_readl() here.

Both sifive_prci_pcie_aux_clock_enable() and
sifive_prci_pcie_aux_clock_is_enabled() are used via the clk_ops
function pointers.

Maybe sifive_prci_pcie_aux_clock_is_enabled() could be replaced by the
__prci_readl() here, but I don't know enough about clk_ops internals
to know.

Bjorn

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

* Re: [PATCH v6 1/6] clk: sifive: Add pcie_aux clock in prci driver for PCIe driver
  2021-05-04 16:23     ` Bjorn Helgaas
@ 2021-05-04 18:12       ` Leon Romanovsky
  2021-05-04 18:45         ` Bjorn Helgaas
  0 siblings, 1 reply; 17+ messages in thread
From: Leon Romanovsky @ 2021-05-04 18:12 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Greentime Hu, paul.walmsley, hes, erik.danie, zong.li, bhelgaas,
	robh+dt, aou, mturquette, sboyd, lorenzo.pieralisi, p.zabel,
	alex.dewar90, khilman, hayashi.kunihiko, vidyas, jh80.chung,
	linux-pci, devicetree, linux-riscv, linux-kernel, linux-clk

On Tue, May 04, 2021 at 11:23:31AM -0500, Bjorn Helgaas wrote:
> On Tue, May 04, 2021 at 03:24:19PM +0300, Leon Romanovsky wrote:
> > On Tue, May 04, 2021 at 06:59:35PM +0800, Greentime Hu wrote:
> > > We add pcie_aux clock in this patch so that pcie driver can use
> > > clk_prepare_enable() and clk_disable_unprepare() to enable and disable
> > > pcie_aux clock.
> > > 
> > > Signed-off-by: Greentime Hu <greentime.hu@sifive.com>
> > > Acked-by: Stephen Boyd <sboyd@kernel.org>
> > > ---
> > >  drivers/clk/sifive/fu740-prci.c               | 11 +++++
> > >  drivers/clk/sifive/fu740-prci.h               |  2 +-
> > >  drivers/clk/sifive/sifive-prci.c              | 41 +++++++++++++++++++
> > >  drivers/clk/sifive/sifive-prci.h              |  9 ++++
> > >  include/dt-bindings/clock/sifive-fu740-prci.h |  1 +
> > >  5 files changed, 63 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/clk/sifive/fu740-prci.c b/drivers/clk/sifive/fu740-prci.c
> > > index 764d1097aa51..53f6e00a03b9 100644
> > > --- a/drivers/clk/sifive/fu740-prci.c
> > > +++ b/drivers/clk/sifive/fu740-prci.c
> > > @@ -72,6 +72,12 @@ static const struct clk_ops sifive_fu740_prci_hfpclkplldiv_clk_ops = {
> > >  	.recalc_rate = sifive_prci_hfpclkplldiv_recalc_rate,
> > 
> > <...>
> > 
> > > +/* PCIE AUX clock APIs for enable, disable. */
> > > +int sifive_prci_pcie_aux_clock_is_enabled(struct clk_hw *hw)
> > 
> > It should be bool
> 
> It's used via this function pointer:
> 
>   struct clk_ops {
>     int             (*is_enabled)(struct clk_hw *hw);
> 
> so I think "int" is actually appropriate here.

Ahh, sorry, I missed that assignment.

> 
> There are some weird/interesting bool vs int usages nearby, though:
> 
>   "bool __is_clk_gate_enabled()" goes to some trouble to convert
>   int to bool ("return (reg_val & bit_mask) != 0;"), and then
>   kona_peri_clk_is_enabled() converts the bool back to int ("return
>   is_clk_gate_enabled(bcm_clk->ccu, gate) ? 1 : 0;").
> 
>   "int lpc32xx_clk_gate_is_enabled()" actually returns a bool that is
>   implicitly converted to int.
> 
>   Many *_is_enabled() functions return !!(...) where !! is an
>   int-to-bool conversion that is arguably unnecessary and again
>   results in an implicit conversion to int.
> 
> I don't see any *problems* with any of these; it just seems like a
> little more mental effort to think about all the explicit and implicit
> conversions going on.

The code is written once but read many times and I can't agree with
your that examples given by you are not the *problems*. They clearly
says "the API is not great and easily can be improved".

Driver authors struggled to write bool-to-int conversion, it is very
optimistic view that they won't struggle to read code too.

Thanks

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

* Re: [PATCH v6 1/6] clk: sifive: Add pcie_aux clock in prci driver for PCIe driver
  2021-05-04 18:12       ` Leon Romanovsky
@ 2021-05-04 18:45         ` Bjorn Helgaas
  2021-05-05  5:22           ` Leon Romanovsky
  0 siblings, 1 reply; 17+ messages in thread
From: Bjorn Helgaas @ 2021-05-04 18:45 UTC (permalink / raw)
  To: Leon Romanovsky
  Cc: Greentime Hu, paul.walmsley, hes, erik.danie, zong.li, bhelgaas,
	robh+dt, aou, mturquette, sboyd, lorenzo.pieralisi, p.zabel,
	alex.dewar90, khilman, hayashi.kunihiko, vidyas, jh80.chung,
	linux-pci, devicetree, linux-riscv, linux-kernel, linux-clk

On Tue, May 04, 2021 at 09:12:57PM +0300, Leon Romanovsky wrote:
> On Tue, May 04, 2021 at 11:23:31AM -0500, Bjorn Helgaas wrote:

> > There are some weird/interesting bool vs int usages nearby, though:
> > 
> >   "bool __is_clk_gate_enabled()" goes to some trouble to convert
> >   int to bool ("return (reg_val & bit_mask) != 0;"), and then
> >   kona_peri_clk_is_enabled() converts the bool back to int ("return
> >   is_clk_gate_enabled(bcm_clk->ccu, gate) ? 1 : 0;").
> > 
> >   "int lpc32xx_clk_gate_is_enabled()" actually returns a bool that is
> >   implicitly converted to int.
> > 
> >   Many *_is_enabled() functions return !!(...) where !! is an
> >   int-to-bool conversion that is arguably unnecessary and again
> >   results in an implicit conversion to int.
> > 
> > I don't see any *problems* with any of these; it just seems like a
> > little more mental effort to think about all the explicit and implicit
> > conversions going on.
> 
> The code is written once but read many times and I can't agree with
> your that examples given by you are not the *problems*. They clearly
> says "the API is not great and easily can be improved".

I certainly agree that it's easier for readers if the style is
consistent.  I just meant I didn't see anything that's an actual bug.  

Bjorn

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

* Re: [PATCH v6 5/6] PCI: fu740: Add SiFive FU740 PCIe host controller driver
  2021-05-04 13:46   ` Bjorn Helgaas
  2021-05-04 14:23     ` Lorenzo Pieralisi
@ 2021-05-05  4:26     ` Greentime Hu
  2021-05-05 14:37       ` Bjorn Helgaas
  1 sibling, 1 reply; 17+ messages in thread
From: Greentime Hu @ 2021-05-05  4:26 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Paul Walmsley, hes, Erik Danie, Zong Li, Bjorn Helgaas, robh+dt,
	Albert Ou, Michael Turquette, Stephen Boyd, Lorenzo Pieralisi,
	Philipp Zabel, alex.dewar90, khilman, hayashi.kunihiko, vidyas,
	jh80.chung, linux-pci, devicetree, linux-riscv,
	Linux Kernel Mailing List, linux-clk

Bjorn Helgaas <helgaas@kernel.org> 於 2021年5月4日 週二 下午9:46寫道:
>
> On Tue, May 04, 2021 at 06:59:39PM +0800, Greentime Hu wrote:
> > From: Paul Walmsley <paul.walmsley@sifive.com>
> >
> > Add driver for the SiFive FU740 PCIe host controller.
> > This controller is based on the DesignWare PCIe core.
> >
> > Signed-off-by: Paul Walmsley <paul.walmsley@sifive.com>
> > Co-developed-by: Henry Styles <hes@sifive.com>
> > Signed-off-by: Henry Styles <hes@sifive.com>
> > Co-developed-by: Erik Danie <erik.danie@sifive.com>
> > Signed-off-by: Erik Danie <erik.danie@sifive.com>
> > Co-developed-by: Greentime Hu <greentime.hu@sifive.com>
> > Signed-off-by: Greentime Hu <greentime.hu@sifive.com>
> > ---
> >  drivers/pci/controller/dwc/Kconfig      |  10 +
> >  drivers/pci/controller/dwc/Makefile     |   1 +
> >  drivers/pci/controller/dwc/pcie-fu740.c | 309 ++++++++++++++++++++++++
> >  3 files changed, 320 insertions(+)
> >  create mode 100644 drivers/pci/controller/dwc/pcie-fu740.c
> >
> > diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
> > index 22c5529e9a65..255d43b1661b 100644
> > --- a/drivers/pci/controller/dwc/Kconfig
> > +++ b/drivers/pci/controller/dwc/Kconfig
> > @@ -318,4 +318,14 @@ 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_FU740
> > +     bool "SiFive FU740 PCIe host controller"
> > +     depends on PCI_MSI_IRQ_DOMAIN
> > +     depends on SOC_SIFIVE || COMPILE_TEST
> > +     depends on GPIOLIB
>
> 1) I'm a little disappointed that I reported the build issue 6 days
>    ago when we were already in the merge window, and it's taken until
>    now to make some progress.
>
> 2) I would prefer not to depend on GPIOLIB because it reduces
>    compile-test coverage.  For example, the x86_64 defconfig does not
>    enable GPIOLIB, so one must manually enable it to even be able to
>    enable PCIE_FU740.
>
>    Many other PCI controller drivers use GPIO, but no others depend on
>    GPIOLIB, so I infer that in the !GPIOLIB case, gpio/consumer.h
>    provides the stubs required for compile testing.
>
>    We could have a conversation about whether it's better to
>    explicitly depend on GPIOLIB here, or whether building a working
>    FU740 driver implicitly depends on GPIOLIB being selected
>    elsewhere.  That implicit dependency *is* a little obscure, but I
>    think that's what other drivers currently do, and I'd like to do
>    this consistently unless there's a good reason otherwise.
>
>    Here are some examples of other drivers:
>
>    dwc/pci-dra7xx.c:
>      config PCI_DRA7XX_HOST
>        depends on SOC_DRA7XX || COMPILE_TEST
>
>      config SOC_DRA7XX
>        select ARCH_OMAP2PLUS
>
>      config ARCH_OMAP2PLUS
>        select GPIOLIB
>
>    dwc/pci-meson.c:
>      config PCI_MESON
>        # doesn't, but probably *should* depend on "ARCH_MESON || COMPILE_TEST"
>
>      menuconfig ARCH_MESON
>        select GPIOLIB
>
>    dwc/pcie-qcom.c:
>      config PCIE_QCOM
>        depends on OF && (ARCH_QCOM || COMPILE_TEST)
>
>      config ARCH_QCOM
>        select GPIOLIB
>
>    pcie-rockchip.c:
>      config PCIE_ROCKCHIP_HOST
>        depends on ARCH_ROCKCHIP || COMPILE_TEST
>
>      config ARCH_ROCKCHIP
>        select GPIOLIB
>
> > +     select PCIE_DW_HOST
> > +     help
> > +       Say Y here if you want PCIe controller support for the SiFive
> > +       FU740.
> > +
> >  endmenu

Hi,

Sorry for late to debug this case. I was working on other works and
just missed the email.
How about this?

diff --git a/arch/riscv/Kconfig.socs b/arch/riscv/Kconfig.socs
index e1b2690b6e45..66f57f2db49d 100644
--- a/arch/riscv/Kconfig.socs
+++ b/arch/riscv/Kconfig.socs
@@ -7,6 +7,7 @@ config SOC_SIFIVE
        select CLK_SIFIVE
        select CLK_SIFIVE_PRCI
        select SIFIVE_PLIC
+       select GPIOLIB if PCIE_FU740
        help
          This enables support for SiFive SoC platform hardware.

diff --git a/drivers/pci/controller/dwc/Kconfig
b/drivers/pci/controller/dwc/Kconfig
index 255d43b1661b..0a37d21ed64e 100644
--- a/drivers/pci/controller/dwc/Kconfig
+++ b/drivers/pci/controller/dwc/Kconfig
@@ -322,7 +322,6 @@ config PCIE_FU740
        bool "SiFive FU740 PCIe host controller"
        depends on PCI_MSI_IRQ_DOMAIN
        depends on SOC_SIFIVE || COMPILE_TEST
-       depends on GPIOLIB
        select PCIE_DW_HOST
        help
          Say Y here if you want PCIe controller support for the SiFive

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

* Re: [PATCH v6 1/6] clk: sifive: Add pcie_aux clock in prci driver for PCIe driver
  2021-05-04 18:45         ` Bjorn Helgaas
@ 2021-05-05  5:22           ` Leon Romanovsky
  0 siblings, 0 replies; 17+ messages in thread
From: Leon Romanovsky @ 2021-05-05  5:22 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Greentime Hu, paul.walmsley, hes, erik.danie, zong.li, bhelgaas,
	robh+dt, aou, mturquette, sboyd, lorenzo.pieralisi, p.zabel,
	alex.dewar90, khilman, hayashi.kunihiko, vidyas, jh80.chung,
	linux-pci, devicetree, linux-riscv, linux-kernel, linux-clk

On Tue, May 04, 2021 at 01:45:55PM -0500, Bjorn Helgaas wrote:
> On Tue, May 04, 2021 at 09:12:57PM +0300, Leon Romanovsky wrote:
> > On Tue, May 04, 2021 at 11:23:31AM -0500, Bjorn Helgaas wrote:
> 
> > > There are some weird/interesting bool vs int usages nearby, though:
> > > 
> > >   "bool __is_clk_gate_enabled()" goes to some trouble to convert
> > >   int to bool ("return (reg_val & bit_mask) != 0;"), and then
> > >   kona_peri_clk_is_enabled() converts the bool back to int ("return
> > >   is_clk_gate_enabled(bcm_clk->ccu, gate) ? 1 : 0;").
> > > 
> > >   "int lpc32xx_clk_gate_is_enabled()" actually returns a bool that is
> > >   implicitly converted to int.
> > > 
> > >   Many *_is_enabled() functions return !!(...) where !! is an
> > >   int-to-bool conversion that is arguably unnecessary and again
> > >   results in an implicit conversion to int.
> > > 
> > > I don't see any *problems* with any of these; it just seems like a
> > > little more mental effort to think about all the explicit and implicit
> > > conversions going on.
> > 
> > The code is written once but read many times and I can't agree with
> > your that examples given by you are not the *problems*. They clearly
> > says "the API is not great and easily can be improved".
> 
> I certainly agree that it's easier for readers if the style is
> consistent.  I just meant I didn't see anything that's an actual bug.  

No one said that it is a bug. My comment is better seen as s suggestion
to the maintainers on how other subsystems keep their code base clean
and up-to date.

Once "the problem" is spotted, the submitter is asked to fix it globally
including fixing other drivers if needed, before "new feature" can be merged.

Of course, there are exceptions from this rule, but they are rare and
usually given to the people who has proven record.

Thanks

> 
> Bjorn

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

* Re: [PATCH v6 5/6] PCI: fu740: Add SiFive FU740 PCIe host controller driver
  2021-05-05  4:26     ` Greentime Hu
@ 2021-05-05 14:37       ` Bjorn Helgaas
  0 siblings, 0 replies; 17+ messages in thread
From: Bjorn Helgaas @ 2021-05-05 14:37 UTC (permalink / raw)
  To: Greentime Hu
  Cc: Paul Walmsley, hes, Erik Danie, Zong Li, Bjorn Helgaas, robh+dt,
	Albert Ou, Michael Turquette, Stephen Boyd, Lorenzo Pieralisi,
	Philipp Zabel, alex.dewar90, khilman, hayashi.kunihiko, vidyas,
	jh80.chung, linux-pci, devicetree, linux-riscv,
	Linux Kernel Mailing List, linux-clk

On Wed, May 05, 2021 at 12:26:31PM +0800, Greentime Hu wrote:
> Bjorn Helgaas <helgaas@kernel.org> 於 2021年5月4日 週二 下午9:46寫道:
> > On Tue, May 04, 2021 at 06:59:39PM +0800, Greentime Hu wrote:
> > > From: Paul Walmsley <paul.walmsley@sifive.com>
> > >
> > > Add driver for the SiFive FU740 PCIe host controller.
> > > This controller is based on the DesignWare PCIe core.
> > >
> > > Signed-off-by: Paul Walmsley <paul.walmsley@sifive.com>
> > > Co-developed-by: Henry Styles <hes@sifive.com>
> > > Signed-off-by: Henry Styles <hes@sifive.com>
> > > Co-developed-by: Erik Danie <erik.danie@sifive.com>
> > > Signed-off-by: Erik Danie <erik.danie@sifive.com>
> > > Co-developed-by: Greentime Hu <greentime.hu@sifive.com>
> > > Signed-off-by: Greentime Hu <greentime.hu@sifive.com>
> > > ---
> > >  drivers/pci/controller/dwc/Kconfig      |  10 +
> > >  drivers/pci/controller/dwc/Makefile     |   1 +
> > >  drivers/pci/controller/dwc/pcie-fu740.c | 309 ++++++++++++++++++++++++
> > >  3 files changed, 320 insertions(+)
> > >  create mode 100644 drivers/pci/controller/dwc/pcie-fu740.c
> > >
> > > diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
> > > index 22c5529e9a65..255d43b1661b 100644
> > > --- a/drivers/pci/controller/dwc/Kconfig
> > > +++ b/drivers/pci/controller/dwc/Kconfig
> > > @@ -318,4 +318,14 @@ 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_FU740
> > > +     bool "SiFive FU740 PCIe host controller"
> > > +     depends on PCI_MSI_IRQ_DOMAIN
> > > +     depends on SOC_SIFIVE || COMPILE_TEST
> > > +     depends on GPIOLIB
> > ...
> > 2) I would prefer not to depend on GPIOLIB because it reduces
> >    compile-test coverage.  For example, the x86_64 defconfig does not
> >    enable GPIOLIB, so one must manually enable it to even be able to
> >    enable PCIE_FU740.
> > ...

> Sorry for late to debug this case. I was working on other works and
> just missed the email.
> How about this?

We already dropped the "depends on GPIOLIB" for v5.13.

You can add a select later, for v5.14.  Of course, you should post a
complete patch including commit log and signed-off-by.

And please take a look at how other drivers handle this so you do it
the same way.

> diff --git a/arch/riscv/Kconfig.socs b/arch/riscv/Kconfig.socs
> index e1b2690b6e45..66f57f2db49d 100644
> --- a/arch/riscv/Kconfig.socs
> +++ b/arch/riscv/Kconfig.socs
> @@ -7,6 +7,7 @@ config SOC_SIFIVE
>         select CLK_SIFIVE
>         select CLK_SIFIVE_PRCI
>         select SIFIVE_PLIC
> +       select GPIOLIB if PCIE_FU740
>         help
>           This enables support for SiFive SoC platform hardware.
> 
> diff --git a/drivers/pci/controller/dwc/Kconfig
> b/drivers/pci/controller/dwc/Kconfig
> index 255d43b1661b..0a37d21ed64e 100644
> --- a/drivers/pci/controller/dwc/Kconfig
> +++ b/drivers/pci/controller/dwc/Kconfig
> @@ -322,7 +322,6 @@ config PCIE_FU740
>         bool "SiFive FU740 PCIe host controller"
>         depends on PCI_MSI_IRQ_DOMAIN
>         depends on SOC_SIFIVE || COMPILE_TEST
> -       depends on GPIOLIB
>         select PCIE_DW_HOST
>         help
>           Say Y here if you want PCIe controller support for the SiFive

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

end of thread, other threads:[~2021-05-05 14:37 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-04 10:59 [PATCH v6 0/6] Add SiFive FU740 PCIe host controller driver support Greentime Hu
2021-05-04 10:59 ` [PATCH v6 1/6] clk: sifive: Add pcie_aux clock in prci driver for PCIe driver Greentime Hu
2021-05-04 12:24   ` Leon Romanovsky
2021-05-04 16:23     ` Bjorn Helgaas
2021-05-04 18:12       ` Leon Romanovsky
2021-05-04 18:45         ` Bjorn Helgaas
2021-05-05  5:22           ` Leon Romanovsky
2021-05-04 10:59 ` [PATCH v6 2/6] clk: sifive: Use reset-simple " Greentime Hu
2021-05-04 10:59 ` [PATCH v6 3/6] MAINTAINERS: Add maintainers for SiFive FU740 " Greentime Hu
2021-05-04 10:59 ` [PATCH v6 4/6] dt-bindings: PCI: Add SiFive FU740 PCIe host controller Greentime Hu
2021-05-04 10:59 ` [PATCH v6 5/6] PCI: fu740: Add SiFive FU740 PCIe host controller driver Greentime Hu
2021-05-04 13:46   ` Bjorn Helgaas
2021-05-04 14:23     ` Lorenzo Pieralisi
2021-05-05  4:26     ` Greentime Hu
2021-05-05 14:37       ` Bjorn Helgaas
2021-05-04 10:59 ` [PATCH v6 6/6] riscv: dts: Add PCIe support for the SiFive FU740-C000 SoC Greentime Hu
2021-05-04 11:28 ` [PATCH v6 0/6] Add SiFive FU740 PCIe host controller driver support Lorenzo Pieralisi

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