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
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 <[email protected]>
Acked-by: Stephen Boyd <[email protected]>
---
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
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 <[email protected]>
Reviewed-by: Philipp Zabel <[email protected]>
Acked-by: Stephen Boyd <[email protected]>
---
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
Add PCIe host controller DT bindings of SiFive FU740.
Signed-off-by: Greentime Hu <[email protected]>
Reviewed-by: Rob Herring <[email protected]>
---
.../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 <[email protected]>
+ - Greentime Hu <[email protected]>
+
+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
From: Paul Walmsley <[email protected]>
Add driver for the SiFive FU740 PCIe host controller.
This controller is based on the DesignWare PCIe core.
Signed-off-by: Paul Walmsley <[email protected]>
Co-developed-by: Henry Styles <[email protected]>
Signed-off-by: Henry Styles <[email protected]>
Co-developed-by: Erik Danie <[email protected]>
Signed-off-by: Erik Danie <[email protected]>
Co-developed-by: Greentime Hu <[email protected]>
Signed-off-by: Greentime Hu <[email protected]>
---
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
Here add maintainer information for SiFive FU740 PCIe driver.
Signed-off-by: Greentime Hu <[email protected]>
---
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 <[email protected]>
+M: Greentime Hu <[email protected]>
+L: [email protected]
+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 <[email protected]>
L: [email protected]
--
2.31.1
Signed-off-by: Greentime Hu <[email protected]>
Acked-by: Palmer Dabbelt <[email protected]>
---
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
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
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 <[email protected]>
> Acked-by: Stephen Boyd <[email protected]>
> ---
> 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
On Tue, May 04, 2021 at 06:59:39PM +0800, Greentime Hu wrote:
> From: Paul Walmsley <[email protected]>
>
> Add driver for the SiFive FU740 PCIe host controller.
> This controller is based on the DesignWare PCIe core.
>
> Signed-off-by: Paul Walmsley <[email protected]>
> Co-developed-by: Henry Styles <[email protected]>
> Signed-off-by: Henry Styles <[email protected]>
> Co-developed-by: Erik Danie <[email protected]>
> Signed-off-by: Erik Danie <[email protected]>
> Co-developed-by: Greentime Hu <[email protected]>
> Signed-off-by: Greentime Hu <[email protected]>
> ---
> 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
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 <[email protected]>
> >
> > Add driver for the SiFive FU740 PCIe host controller.
> > This controller is based on the DesignWare PCIe core.
> >
> > Signed-off-by: Paul Walmsley <[email protected]>
> > Co-developed-by: Henry Styles <[email protected]>
> > Signed-off-by: Henry Styles <[email protected]>
> > Co-developed-by: Erik Danie <[email protected]>
> > Signed-off-by: Erik Danie <[email protected]>
> > Co-developed-by: Greentime Hu <[email protected]>
> > Signed-off-by: Greentime Hu <[email protected]>
> > ---
> > 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
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 <[email protected]>
> > Acked-by: Stephen Boyd <[email protected]>
> > ---
> > 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
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 <[email protected]>
> > > Acked-by: Stephen Boyd <[email protected]>
> > > ---
> > > 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
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
Bjorn Helgaas <[email protected]> 於 2021年5月4日 週二 下午9:46寫道：
>
> On Tue, May 04, 2021 at 06:59:39PM +0800, Greentime Hu wrote:
> > From: Paul Walmsley <[email protected]>
> >
> > Add driver for the SiFive FU740 PCIe host controller.
> > This controller is based on the DesignWare PCIe core.
> >
> > Signed-off-by: Paul Walmsley <[email protected]>
> > Co-developed-by: Henry Styles <[email protected]>
> > Signed-off-by: Henry Styles <[email protected]>
> > Co-developed-by: Erik Danie <[email protected]>
> > Signed-off-by: Erik Danie <[email protected]>
> > Co-developed-by: Greentime Hu <[email protected]>
> > Signed-off-by: Greentime Hu <[email protected]>
> > ---
> > 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
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
On Wed, May 05, 2021 at 12:26:31PM +0800, Greentime Hu wrote:
> Bjorn Helgaas <[email protected]> 於 2021年5月4日 週二 下午9:46寫道：
> > On Tue, May 04, 2021 at 06:59:39PM +0800, Greentime Hu wrote:
> > > From: Paul Walmsley <[email protected]>
> > >
> > > Add driver for the SiFive FU740 PCIe host controller.
> > > This controller is based on the DesignWare PCIe core.
> > >
> > > Signed-off-by: Paul Walmsley <[email protected]>
> > > Co-developed-by: Henry Styles <[email protected]>
> > > Signed-off-by: Henry Styles <[email protected]>
> > > Co-developed-by: Erik Danie <[email protected]>
> > > Signed-off-by: Erik Danie <[email protected]>
> > > Co-developed-by: Greentime Hu <[email protected]>
> > > Signed-off-by: Greentime Hu <[email protected]>
> > > ---
> > > 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