Hi,
This series is the PCIe driver for Toshiba's ARM SoC, Visconti[0].
This provides DT binding documentation, device driver, MAINTAINER files.
Best regards,
Nobuhiro
[0]: https://toshiba.semicon-storage.com/ap-en/semiconductor/product/image-recognition-processors-visconti.html
dt-bindings: pci: Add DT binding for Toshiba Visconti PCIe controller
v1 -> v2:
- Remove white space.
- Drop num-viewport and bus-range from required.
- Drop status line from example.
- Drop bus-range from required.
- Removed lines defined in pci-bus.yaml from required.
PCI: dwc: Visconti: PCIe RC controller driver
v1 -> v2:
- Fix typo in commit message.
- Drop "depends on OF && HAS_IOMEM" from Kconfig.
- Stop using the pointer of struct dw_pcie.
- Use _relaxed variant.
- Drop dw_pcie_wait_for_link.
- Drop dbi resource processing.
- Drop MSI IRQ initialization processing.
MAINTAINERS: Add entries for Toshiba Visconti PCIe controller
v1 -> v2:
- No update.
Nobuhiro Iwamatsu (3):
dt-bindings: pci: Add DT binding for Toshiba Visconti PCIe controller
PCI: dwc: Visconti: PCIe RC controller driver
MAINTAINERS: Add entries for Toshiba Visconti PCIe controller
.../bindings/pci/toshiba,visconti-pcie.yaml | 110 ++++++
MAINTAINERS | 2 +
drivers/pci/controller/dwc/Kconfig | 9 +
drivers/pci/controller/dwc/Makefile | 1 +
drivers/pci/controller/dwc/pcie-visconti.c | 333 ++++++++++++++++++
5 files changed, 455 insertions(+)
create mode 100644 Documentation/devicetree/bindings/pci/toshiba,visconti-pcie.yaml
create mode 100644 drivers/pci/controller/dwc/pcie-visconti.c
--
2.30.0.rc2
This commit adds the Device Tree binding documentation that allows
to describe the PCIe controller found in Toshiba Visconti SoCs.
Signed-off-by: Nobuhiro Iwamatsu <[email protected]>
---
.../bindings/pci/toshiba,visconti-pcie.yaml | 110 ++++++++++++++++++
1 file changed, 110 insertions(+)
create mode 100644 Documentation/devicetree/bindings/pci/toshiba,visconti-pcie.yaml
diff --git a/Documentation/devicetree/bindings/pci/toshiba,visconti-pcie.yaml b/Documentation/devicetree/bindings/pci/toshiba,visconti-pcie.yaml
new file mode 100644
index 000000000000..d47a4a3c49e3
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/toshiba,visconti-pcie.yaml
@@ -0,0 +1,110 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pci/toshiba,visconti-pcie.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Toshiba Visconti5 SoC PCIe Host Controller Device Tree Bindings
+
+maintainers:
+ - Nobuhiro Iwamatsu <[email protected]>
+
+description: |+
+ Toshiba Visconti5 SoC PCIe host controller is based on the Synopsys DesignWare PCIe IP.
+
+allOf:
+ - $ref: /schemas/pci/pci-bus.yaml#
+
+properties:
+ compatible:
+ const: toshiba,visconti-pcie
+
+ reg:
+ items:
+ - description: Data Bus Interface (DBI) registers.
+ - description: PCIe configuration space region.
+ - description: Visconti specific additional registers.
+ - description: Visconti specific SMU registers
+ - description: Visconti specific memory protection unit registers (MPU)
+
+ reg-names:
+ items:
+ - const: dbi
+ - const: config
+ - const: ulreg
+ - const: smu
+ - const: mpu
+
+ interrupts:
+ maxItems: 1
+
+ clocks:
+ items:
+ - description: PCIe reference clock
+ - description: PCIe system clock
+ - description: Auxiliary clock
+
+ clock-names:
+ items:
+ - const: pcie_refclk
+ - const: sysclk
+ - const: auxclk
+
+ num-lanes:
+ const: 2
+
+required:
+ - reg
+ - reg-names
+ - interrupts
+ - "#interrupt-cells"
+ - interrupt-map
+ - interrupt-map-mask
+ - num-lanes
+ - clocks
+ - clock-names
+ - max-link-speed
+
+unevaluatedProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/interrupt-controller/irq.h>
+ #include <dt-bindings/interrupt-controller/arm-gic.h>
+
+ soc {
+ #address-cells = <2>;
+ #size-cells = <2>;
+
+ pcie: pcie@28400000 {
+ compatible = "toshiba,visconti-pcie";
+ reg = <0x0 0x28400000 0x0 0x00400000>,
+ <0x0 0x70000000 0x0 0x10000000>,
+ <0x0 0x28050000 0x0 0x00010000>,
+ <0x0 0x24200000 0x0 0x00002000>,
+ <0x0 0x24162000 0x0 0x00001000>;
+ reg-names = "dbi", "config", "ulreg", "smu", "mpu";
+ device_type = "pci";
+ bus-range = <0x00 0xff>;
+ num-lanes = <2>;
+ num-viewport = <8>;
+
+ #address-cells = <3>;
+ #size-cells = <2>;
+ #interrupt-cells = <1>;
+ ranges = <0x81000000 0 0x40000000 0 0x40000000 0 0x00010000>,
+ <0x82000000 0 0x50000000 0 0x50000000 0 0x20000000>;
+ interrupts = <GIC_SPI 215 IRQ_TYPE_LEVEL_HIGH>;
+ interrupt-names = "intr";
+ interrupt-map-mask = <0 0 0 7>;
+ interrupt-map =
+ <0 0 0 1 &gic GIC_SPI 215 IRQ_TYPE_LEVEL_HIGH
+ 0 0 0 2 &gic GIC_SPI 215 IRQ_TYPE_LEVEL_HIGH
+ 0 0 0 3 &gic GIC_SPI 215 IRQ_TYPE_LEVEL_HIGH
+ 0 0 0 4 &gic GIC_SPI 215 IRQ_TYPE_LEVEL_HIGH>;
+ clocks = <&extclk100mhz>, <&clk600mhz>, <&clk25mhz>;
+ clock-names = "pcie_refclk", "sysclk", "auxclk";
+ max-link-speed = <2>;
+ };
+ };
+...
--
2.30.0.rc2
Add entries for Toshiba Visconti PCIe controller binding and driver.
Signed-off-by: Nobuhiro Iwamatsu <[email protected]>
---
MAINTAINERS | 2 ++
1 file changed, 2 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index 8a154939ae27..3e5187c5b8d2 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2621,11 +2621,13 @@ T: git git://git.kernel.org/pub/scm/linux/kernel/git/iwamatsu/linux-visconti.git
F: Documentation/devicetree/bindings/arm/toshiba.yaml
F: Documentation/devicetree/bindings/net/toshiba,visconti-dwmac.yaml
F: Documentation/devicetree/bindings/gpio/toshiba,gpio-visconti.yaml
+F: Documentation/devicetree/bindings/pci/toshiba,visconti-pcie.yaml
F: Documentation/devicetree/bindings/pinctrl/toshiba,tmpv7700-pinctrl.yaml
F: Documentation/devicetree/bindings/watchdog/toshiba,visconti-wdt.yaml
F: arch/arm64/boot/dts/toshiba/
F: drivers/net/ethernet/stmicro/stmmac/dwmac-visconti.c
F: drivers/gpio/gpio-visconti.c
+F: drivers/pci/controller/dwc/pcie-visconti.c
F: drivers/pinctrl/visconti/
F: drivers/watchdog/visconti_wdt.c
N: visconti
--
2.30.0.rc2
Add support to PCIe RC controller on Toshiba Visconti ARM SoCs. PCIe controller is based of Synopsys
DesignWare PCIe core.
This patch does not yet use the clock framework to control the clock. This will be replaced in the
future.
Signed-off-by: Yuji Ishikawa <[email protected]>
Signed-off-by: Nobuhiro Iwamatsu <[email protected]>
---
drivers/pci/controller/dwc/Kconfig | 9 +
drivers/pci/controller/dwc/Makefile | 1 +
drivers/pci/controller/dwc/pcie-visconti.c | 333 +++++++++++++++++++++
3 files changed, 343 insertions(+)
create mode 100644 drivers/pci/controller/dwc/pcie-visconti.c
diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
index b9aaa84452c4..043c2c70d566 100644
--- a/drivers/pci/controller/dwc/Kconfig
+++ b/drivers/pci/controller/dwc/Kconfig
@@ -286,6 +286,15 @@ config PCIE_TEGRA194_EP
in order to enable device-specific features PCIE_TEGRA194_EP must be
selected. This uses the DesignWare core.
+config PCIE_VISCONTI
+ bool "Toshiba VISCONTI PCIe controllers"
+ depends on ARCH_VISCONTI || COMPILE_TEST
+ depends on PCI_MSI_IRQ_DOMAIN
+ select PCIE_DW_HOST
+ help
+ Say Y here if you want PCIe controller support on Toshiba Visconti SoC.
+ This driver supports TMPV77xx.
+
config PCIE_UNIPHIER
bool "Socionext UniPhier PCIe host controllers"
depends on ARCH_UNIPHIER || COMPILE_TEST
diff --git a/drivers/pci/controller/dwc/Makefile b/drivers/pci/controller/dwc/Makefile
index ba7c42f6df6f..46ac5d49dc75 100644
--- a/drivers/pci/controller/dwc/Makefile
+++ b/drivers/pci/controller/dwc/Makefile
@@ -20,6 +20,7 @@ obj-$(CONFIG_PCI_MESON) += pci-meson.o
obj-$(CONFIG_PCIE_TEGRA194) += pcie-tegra194.o
obj-$(CONFIG_PCIE_UNIPHIER) += pcie-uniphier.o
obj-$(CONFIG_PCIE_UNIPHIER_EP) += pcie-uniphier-ep.o
+obj-$(CONFIG_PCIE_VISCONTI) += pcie-visconti.o
# The following drivers are for devices that use the generic ACPI
# pci_root.c driver but don't support standard ECAM config access.
diff --git a/drivers/pci/controller/dwc/pcie-visconti.c b/drivers/pci/controller/dwc/pcie-visconti.c
new file mode 100644
index 000000000000..a4739e7187f0
--- /dev/null
+++ b/drivers/pci/controller/dwc/pcie-visconti.c
@@ -0,0 +1,333 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * DWC PCIe RC driver for Toshiba Visconti ARM SoC
+ *
+ * Copyright (C) 2019, 2020 Toshiba Electronic Device & Storage Corporation
+ * Copyright (C) 2020, TOSHIBA CORPORATION
+ *
+ * Nobuhiro Iwamatsu <[email protected]>
+ *
+ */
+
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/gpio.h>
+#include <linux/interrupt.h>
+#include <linux/init.h>
+#include <linux/iopoll.h>
+#include <linux/kernel.h>
+#include <linux/of_platform.h>
+#include <linux/pci.h>
+#include <linux/platform_device.h>
+#include <linux/resource.h>
+#include <linux/types.h>
+
+#include "pcie-designware.h"
+#include "../../pci.h"
+
+struct visconti_pcie {
+ struct dw_pcie pci;
+ void __iomem *ulreg_base;
+ void __iomem *smu_base;
+ void __iomem *mpu_base;
+ struct clk *refclk;
+ struct clk *sysclk;
+ struct clk *auxclk;
+};
+
+#define PCIE_UL_REG_S_PCIE_MODE 0x00F4
+#define PCIE_UL_REG_S_PCIE_MODE_EP 0x00
+#define PCIE_UL_REG_S_PCIE_MODE_RC 0x04
+
+#define PCIE_UL_REG_S_PERSTN_CTRL 0x00F8
+#define PCIE_UL_IOM_PCIE_PERSTN_I_EN BIT(3)
+#define PCIE_UL_DIRECT_PERSTN_EN BIT(2)
+#define PCIE_UL_PERSTN_OUT BIT(1)
+#define PCIE_UL_DIRECT_PERSTN BIT(0)
+
+#define PCIE_UL_REG_S_PHY_INIT_02 0x0104
+#define PCIE_UL_PHY0_SRAM_EXT_LD_DONE BIT(0)
+
+#define PCIE_UL_REG_S_PHY_INIT_03 0x0108
+#define PCIE_UL_PHY0_SRAM_INIT_DONE BIT(0)
+
+#define PCIE_UL_REG_S_INT_EVENT_MASK1 0x0138
+#define PCIE_UL_CFG_PME_INT BIT(0)
+#define PCIE_UL_CFG_LINK_EQ_REQ_INT BIT(1)
+#define PCIE_UL_EDMA_INT0 BIT(2)
+#define PCIE_UL_EDMA_INT1 BIT(3)
+#define PCIE_UL_EDMA_INT2 BIT(4)
+#define PCIE_UL_EDMA_INT3 BIT(5)
+#define PCIE_UL_S_INT_EVENT_MASK1_ALL (PCIE_UL_CFG_PME_INT | PCIE_UL_CFG_LINK_EQ_REQ_INT | \
+ PCIE_UL_EDMA_INT0 | PCIE_UL_EDMA_INT1 | \
+ PCIE_UL_EDMA_INT2 | PCIE_UL_EDMA_INT3)
+
+#define PCIE_UL_REG_S_SB_MON 0x0198
+#define PCIE_UL_REG_S_SIG_MON 0x019C
+#define PCIE_UL_CORE_RST_N_MON BIT(0)
+
+#define PCIE_UL_REG_V_SII_DBG_00 0x0844
+#define PCIE_UL_REG_V_SII_GEN_CTRL_01 0x0860
+#define PCIE_UL_APP_LTSSM_ENABLE BIT(0)
+
+#define PCIE_UL_REG_V_PHY_ST_00 0x0864
+#define PCIE_UL_SMLH_LINK_UP BIT(0)
+
+#define PCIE_UL_REG_V_PHY_ST_02 0x0868
+#define PCIE_UL_S_DETECT_ACT 0x01
+#define PCIE_UL_S_L0 0x11
+
+#define PISMU_CKON_PCIE 0x0038
+#define PISMU_CKON_PCIE_AUX_CLK BIT(1)
+#define PISMU_CKON_PCIE_MSTR_ACLK BIT(0)
+
+#define PISMU_RSOFF_PCIE 0x0538
+#define PISMU_RSOFF_PCIE_ULREG_RST_N BIT(1)
+#define PISMU_RSOFF_PCIE_PWR_UP_RST_N BIT(0)
+
+#define PCIE_MPU_REG_MP_EN 0x0
+#define MPU_MP_EN_DISABLE BIT(0)
+
+#define PCIE_BUS_OFFSET 0x40000000
+
+/* Access registers in PCIe ulreg */
+static inline void visconti_ulreg_writel(struct visconti_pcie *pcie, u32 val, u32 reg)
+{
+ writel_relaxed(val, pcie->ulreg_base + reg);
+}
+
+/* Access registers in PCIe smu */
+static inline void visconti_smu_writel(struct visconti_pcie *pcie, u32 val, u32 reg)
+{
+ writel_relaxed(val, pcie->smu_base + reg);
+}
+
+/* Access registers in PCIe mpu */
+static inline void visconti_mpu_writel(struct visconti_pcie *pcie, u32 val, u32 reg)
+{
+ writel_relaxed(val, pcie->mpu_base + reg);
+}
+
+static inline u32 visconti_mpu_readl(struct visconti_pcie *pcie, u32 reg)
+{
+ return readl_relaxed(pcie->mpu_base + reg);
+}
+
+static int visconti_pcie_check_link_status(struct visconti_pcie *pcie)
+{
+ int err;
+ u32 val;
+
+ /* wait for linkup of phy link layer */
+ err = readl_relaxed_poll_timeout(pcie->ulreg_base + PCIE_UL_REG_V_PHY_ST_00, val,
+ (val & PCIE_UL_SMLH_LINK_UP), 1000, 10000);
+ if (err)
+ return err;
+
+ /* wait for linkup of data link layer */
+ err = readl_relaxed_poll_timeout(pcie->ulreg_base + PCIE_UL_REG_V_PHY_ST_02, val,
+ (val & PCIE_UL_S_DETECT_ACT), 1000, 10000);
+ if (err)
+ return err;
+
+ /* wait for LTSSM Status */
+ return readl_relaxed_poll_timeout(pcie->ulreg_base + PCIE_UL_REG_V_PHY_ST_02, val,
+ (val & PCIE_UL_S_L0), 1000, 10000);
+}
+
+static int visconti_pcie_establish_link(struct pcie_port *pp)
+{
+ int ret;
+ u32 val;
+ struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
+ struct visconti_pcie *pcie = dev_get_drvdata(pci->dev);
+
+ visconti_ulreg_writel(pcie, PCIE_UL_APP_LTSSM_ENABLE, PCIE_UL_REG_V_SII_GEN_CTRL_01);
+
+ ret = visconti_pcie_check_link_status(pcie);
+ if (ret < 0) {
+ dev_info(pci->dev, "Link failure\n");
+ return ret;
+ }
+
+ val = visconti_mpu_readl(pcie, PCIE_MPU_REG_MP_EN);
+ visconti_mpu_writel(pcie, val & ~MPU_MP_EN_DISABLE, PCIE_MPU_REG_MP_EN);
+
+ visconti_ulreg_writel(pcie, PCIE_UL_S_INT_EVENT_MASK1_ALL, PCIE_UL_REG_S_INT_EVENT_MASK1);
+
+ return 0;
+}
+
+static int visconti_pcie_host_init(struct pcie_port *pp)
+{
+ dw_pcie_setup_rc(pp);
+ return visconti_pcie_establish_link(pp);
+}
+
+static const struct dw_pcie_host_ops visconti_pcie_host_ops = {
+ .host_init = visconti_pcie_host_init,
+};
+
+static u64 visconti_pcie_cpu_addr_fixup(struct dw_pcie *pci, u64 pci_addr)
+{
+ return pci_addr - PCIE_BUS_OFFSET;
+}
+
+static const struct dw_pcie_ops dw_pcie_ops = {
+ .cpu_addr_fixup = visconti_pcie_cpu_addr_fixup,
+};
+
+static int visconti_get_resources(struct platform_device *pdev,
+ struct visconti_pcie *pcie)
+{
+ struct device *dev = &pdev->dev;
+
+ pcie->ulreg_base = devm_platform_ioremap_resource_byname(pdev, "ulreg");
+ if (IS_ERR(pcie->ulreg_base))
+ return PTR_ERR(pcie->ulreg_base);
+
+ pcie->smu_base = devm_platform_ioremap_resource_byname(pdev, "smu");
+ if (IS_ERR(pcie->smu_base))
+ return PTR_ERR(pcie->smu_base);
+
+ pcie->mpu_base = devm_platform_ioremap_resource_byname(pdev, "mpu");
+ if (IS_ERR(pcie->mpu_base))
+ return PTR_ERR(pcie->mpu_base);
+
+ pcie->refclk = devm_clk_get(dev, "pcie_refclk");
+ if (IS_ERR(pcie->refclk)) {
+ dev_err(dev, "Failed to get refclk clock: %ld\n", PTR_ERR(pcie->refclk));
+ return PTR_ERR(pcie->refclk);
+ }
+
+ pcie->sysclk = devm_clk_get(dev, "sysclk");
+ if (IS_ERR(pcie->sysclk)) {
+ dev_err(dev, "Failed to get sysclk clock: %ld\n", PTR_ERR(pcie->sysclk));
+ return PTR_ERR(pcie->sysclk);
+ }
+
+ pcie->auxclk = devm_clk_get(dev, "auxclk");
+ if (IS_ERR(pcie->auxclk)) {
+ dev_err(dev, "Failed to get auxclk clock: %ld\n", PTR_ERR(pcie->auxclk));
+ return PTR_ERR(pcie->auxclk);
+ }
+
+ return 0;
+}
+
+static int visconti_device_turnon(struct visconti_pcie *pcie)
+{
+ int err;
+ u32 val;
+
+ visconti_smu_writel(pcie, PISMU_CKON_PCIE_AUX_CLK | PISMU_CKON_PCIE_MSTR_ACLK,
+ PISMU_CKON_PCIE);
+ ndelay(250);
+
+ visconti_smu_writel(pcie, PISMU_RSOFF_PCIE_ULREG_RST_N, PISMU_RSOFF_PCIE);
+
+ visconti_ulreg_writel(pcie, PCIE_UL_REG_S_PCIE_MODE_RC, PCIE_UL_REG_S_PCIE_MODE);
+
+ val = PCIE_UL_IOM_PCIE_PERSTN_I_EN | PCIE_UL_DIRECT_PERSTN_EN | PCIE_UL_DIRECT_PERSTN;
+ visconti_ulreg_writel(pcie, val, PCIE_UL_REG_S_PERSTN_CTRL);
+ udelay(100);
+
+ val |= PCIE_UL_PERSTN_OUT;
+ visconti_ulreg_writel(pcie, val, PCIE_UL_REG_S_PERSTN_CTRL);
+ udelay(100);
+
+ visconti_smu_writel(pcie, PISMU_RSOFF_PCIE_PWR_UP_RST_N, PISMU_RSOFF_PCIE);
+
+ err = readl_relaxed_poll_timeout(pcie->ulreg_base + PCIE_UL_REG_S_PHY_INIT_03, val,
+ (val & PCIE_UL_PHY0_SRAM_INIT_DONE), 100, 1000);
+ if (err)
+ return err;
+
+ visconti_ulreg_writel(pcie, PCIE_UL_PHY0_SRAM_EXT_LD_DONE, PCIE_UL_REG_S_PHY_INIT_02);
+
+ return readl_relaxed_poll_timeout(pcie->ulreg_base + PCIE_UL_REG_S_SIG_MON, val,
+ (val & PCIE_UL_CORE_RST_N_MON), 100, 1000);
+}
+
+static int visconti_add_pcie_port(struct visconti_pcie *pcie, struct platform_device *pdev)
+{
+ struct dw_pcie *pci = &pcie->pci;
+ struct pcie_port *pp = &pci->pp;
+ struct device *dev = &pdev->dev;
+ int ret;
+
+ pp->irq = platform_get_irq_byname(pdev, "intr");
+ if (pp->irq < 0) {
+ dev_err(dev, "interrupt intr is missing");
+ return pp->irq;
+ }
+
+ pp->ops = &visconti_pcie_host_ops;
+
+ pci->link_gen = of_pci_get_max_link_speed(pdev->dev.of_node);
+ if (pci->link_gen < 0 || pci->link_gen > 3) {
+ pci->link_gen = 3;
+ dev_dbg(dev, "Applied default link speed\n");
+ }
+
+ dev_dbg(dev, "link speed Gen %d", pci->link_gen);
+
+ ret = visconti_device_turnon(pcie);
+ if (ret)
+ goto error;
+
+ ret = dw_pcie_host_init(pp);
+ if (ret)
+ dev_err(dev, "Failed to initialize host\n");
+
+error:
+ return ret;
+}
+
+static int visconti_pcie_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct visconti_pcie *pcie;
+ struct pcie_port *pp;
+ struct dw_pcie *pci;
+ int ret;
+
+ ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(36));
+ if (ret)
+ return ret;
+
+ pcie = devm_kzalloc(dev, sizeof(*pcie), GFP_KERNEL);
+ if (!pcie)
+ return -ENOMEM;
+
+ pci = &pcie->pci;
+ pp = &pci->pp;
+ pp->num_vectors = MAX_MSI_IRQS;
+
+ pci->dev = dev;
+ pci->ops = &dw_pcie_ops;
+
+ ret = visconti_get_resources(pdev, pcie);
+ if (ret)
+ return ret;
+
+ platform_set_drvdata(pdev, pcie);
+
+ return visconti_add_pcie_port(pcie, pdev);
+}
+
+static const struct of_device_id visconti_pcie_match[] = {
+ { .compatible = "toshiba,visconti-pcie" },
+ {},
+};
+
+static struct platform_driver visconti_pcie_driver = {
+ .probe = visconti_pcie_probe,
+ .driver = {
+ .name = "visconti-pcie",
+ .of_match_table = visconti_pcie_match,
+ .suppress_bind_attrs = true,
+ },
+};
+
+builtin_platform_driver(visconti_pcie_driver);
--
2.30.0.rc2
Please use something like this as the subject to match existing
convention:
PCI: visconti: Add Toshiba Visconti PCIe host controller driver
(You can use "git log --oneline drivers/pci/controller/dwc" to learn
this.)
On Mon, Apr 19, 2021 at 03:35:12PM +0900, Nobuhiro Iwamatsu wrote:
> Add support to PCIe RC controller on Toshiba Visconti ARM SoCs. PCIe controller is based of Synopsys
> DesignWare PCIe core.
>
> This patch does not yet use the clock framework to control the clock. This will be replaced in the
> future.
Wrap this text to fit nicely in 75 columns.
> Signed-off-by: Yuji Ishikawa <[email protected]>
> Signed-off-by: Nobuhiro Iwamatsu <[email protected]>
> ---
> drivers/pci/controller/dwc/Kconfig | 9 +
> drivers/pci/controller/dwc/Makefile | 1 +
> drivers/pci/controller/dwc/pcie-visconti.c | 333 +++++++++++++++++++++
> 3 files changed, 343 insertions(+)
> create mode 100644 drivers/pci/controller/dwc/pcie-visconti.c
>
> diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
> index b9aaa84452c4..043c2c70d566 100644
> --- a/drivers/pci/controller/dwc/Kconfig
> +++ b/drivers/pci/controller/dwc/Kconfig
> @@ -286,6 +286,15 @@ config PCIE_TEGRA194_EP
> in order to enable device-specific features PCIE_TEGRA194_EP must be
> selected. This uses the DesignWare core.
>
> +config PCIE_VISCONTI
Consider using PCIE_VISCONTI_HOST in case you ever adapt this hardware
to work in endpoint mode.
> + bool "Toshiba VISCONTI PCIe controllers"
s/VISCONTI/Visconti/ to match usage below.
> + depends on ARCH_VISCONTI || COMPILE_TEST
> + depends on PCI_MSI_IRQ_DOMAIN
> + select PCIE_DW_HOST
> + help
> + Say Y here if you want PCIe controller support on Toshiba Visconti SoC.
> + This driver supports TMPV77xx.
What is "TMPV77xx"? A SoC? A reference platform?
> config PCIE_UNIPHIER
> bool "Socionext UniPhier PCIe host controllers"
> depends on ARCH_UNIPHIER || COMPILE_TEST
> diff --git a/drivers/pci/controller/dwc/Makefile b/drivers/pci/controller/dwc/Makefile
> index ba7c42f6df6f..46ac5d49dc75 100644
> --- a/drivers/pci/controller/dwc/Makefile
> +++ b/drivers/pci/controller/dwc/Makefile
> @@ -20,6 +20,7 @@ obj-$(CONFIG_PCI_MESON) += pci-meson.o
> obj-$(CONFIG_PCIE_TEGRA194) += pcie-tegra194.o
> obj-$(CONFIG_PCIE_UNIPHIER) += pcie-uniphier.o
> obj-$(CONFIG_PCIE_UNIPHIER_EP) += pcie-uniphier-ep.o
> +obj-$(CONFIG_PCIE_VISCONTI) += pcie-visconti.o
>
> # The following drivers are for devices that use the generic ACPI
> # pci_root.c driver but don't support standard ECAM config access.
> diff --git a/drivers/pci/controller/dwc/pcie-visconti.c b/drivers/pci/controller/dwc/pcie-visconti.c
> new file mode 100644
> index 000000000000..a4739e7187f0
> --- /dev/null
> +++ b/drivers/pci/controller/dwc/pcie-visconti.c
> @@ -0,0 +1,333 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * DWC PCIe RC driver for Toshiba Visconti ARM SoC
> + *
> + * Copyright (C) 2019, 2020 Toshiba Electronic Device & Storage Corporation
> + * Copyright (C) 2020, TOSHIBA CORPORATION
> + *
> + * Nobuhiro Iwamatsu <[email protected]>
> + *
Remove spurious blank line.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/gpio.h>
> +#include <linux/interrupt.h>
> +#include <linux/init.h>
> +#include <linux/iopoll.h>
> +#include <linux/kernel.h>
> +#include <linux/of_platform.h>
> +#include <linux/pci.h>
> +#include <linux/platform_device.h>
> +#include <linux/resource.h>
> +#include <linux/types.h>
> +
> +#include "pcie-designware.h"
> +#include "../../pci.h"
> +
> +struct visconti_pcie {
> + struct dw_pcie pci;
> + void __iomem *ulreg_base;
> + void __iomem *smu_base;
> + void __iomem *mpu_base;
> + struct clk *refclk;
> + struct clk *sysclk;
> + struct clk *auxclk;
> +};
> +
> +#define PCIE_UL_REG_S_PCIE_MODE 0x00F4
> +#define PCIE_UL_REG_S_PCIE_MODE_EP 0x00
> +#define PCIE_UL_REG_S_PCIE_MODE_RC 0x04
> +
> +#define PCIE_UL_REG_S_PERSTN_CTRL 0x00F8
> +#define PCIE_UL_IOM_PCIE_PERSTN_I_EN BIT(3)
> +#define PCIE_UL_DIRECT_PERSTN_EN BIT(2)
> +#define PCIE_UL_PERSTN_OUT BIT(1)
> +#define PCIE_UL_DIRECT_PERSTN BIT(0)
> +
> +#define PCIE_UL_REG_S_PHY_INIT_02 0x0104
> +#define PCIE_UL_PHY0_SRAM_EXT_LD_DONE BIT(0)
> +
> +#define PCIE_UL_REG_S_PHY_INIT_03 0x0108
> +#define PCIE_UL_PHY0_SRAM_INIT_DONE BIT(0)
> +
> +#define PCIE_UL_REG_S_INT_EVENT_MASK1 0x0138
> +#define PCIE_UL_CFG_PME_INT BIT(0)
> +#define PCIE_UL_CFG_LINK_EQ_REQ_INT BIT(1)
> +#define PCIE_UL_EDMA_INT0 BIT(2)
> +#define PCIE_UL_EDMA_INT1 BIT(3)
> +#define PCIE_UL_EDMA_INT2 BIT(4)
> +#define PCIE_UL_EDMA_INT3 BIT(5)
> +#define PCIE_UL_S_INT_EVENT_MASK1_ALL (PCIE_UL_CFG_PME_INT | PCIE_UL_CFG_LINK_EQ_REQ_INT | \
> + PCIE_UL_EDMA_INT0 | PCIE_UL_EDMA_INT1 | \
> + PCIE_UL_EDMA_INT2 | PCIE_UL_EDMA_INT3)
> +
Please wrap the code here and below so it fits nicely in 80 columns.
> +#define PCIE_UL_REG_S_SB_MON 0x0198
> +#define PCIE_UL_REG_S_SIG_MON 0x019C
> +#define PCIE_UL_CORE_RST_N_MON BIT(0)
> +
> +#define PCIE_UL_REG_V_SII_DBG_00 0x0844
> +#define PCIE_UL_REG_V_SII_GEN_CTRL_01 0x0860
> +#define PCIE_UL_APP_LTSSM_ENABLE BIT(0)
> +
> +#define PCIE_UL_REG_V_PHY_ST_00 0x0864
> +#define PCIE_UL_SMLH_LINK_UP BIT(0)
> +
> +#define PCIE_UL_REG_V_PHY_ST_02 0x0868
> +#define PCIE_UL_S_DETECT_ACT 0x01
> +#define PCIE_UL_S_L0 0x11
> +
> +#define PISMU_CKON_PCIE 0x0038
> +#define PISMU_CKON_PCIE_AUX_CLK BIT(1)
> +#define PISMU_CKON_PCIE_MSTR_ACLK BIT(0)
> +
> +#define PISMU_RSOFF_PCIE 0x0538
> +#define PISMU_RSOFF_PCIE_ULREG_RST_N BIT(1)
> +#define PISMU_RSOFF_PCIE_PWR_UP_RST_N BIT(0)
> +
> +#define PCIE_MPU_REG_MP_EN 0x0
> +#define MPU_MP_EN_DISABLE BIT(0)
> +
> +#define PCIE_BUS_OFFSET 0x40000000
> +
> +/* Access registers in PCIe ulreg */
> +static inline void visconti_ulreg_writel(struct visconti_pcie *pcie, u32 val, u32 reg)
> +{
> + writel_relaxed(val, pcie->ulreg_base + reg);
> +}
> +
> +/* Access registers in PCIe smu */
> +static inline void visconti_smu_writel(struct visconti_pcie *pcie, u32 val, u32 reg)
> +{
> + writel_relaxed(val, pcie->smu_base + reg);
> +}
> +
> +/* Access registers in PCIe mpu */
> +static inline void visconti_mpu_writel(struct visconti_pcie *pcie, u32 val, u32 reg)
> +{
> + writel_relaxed(val, pcie->mpu_base + reg);
> +}
> +
> +static inline u32 visconti_mpu_readl(struct visconti_pcie *pcie, u32 reg)
Don't bother with "inline" above and the signature will fit better in
the line. The compiler will inline these even if you don't specify
it.
> +{
> + return readl_relaxed(pcie->mpu_base + reg);
> +}
> +
> +static int visconti_pcie_check_link_status(struct visconti_pcie *pcie)
"check_link_status" gives no hint about what the return value means.
This looks like it might be similar to what other drivers do in
*_pcie_link_up().
Can you hook things up so this generic code in dw_pcie_host_init()
works for you?
if (!dw_pcie_link_up(pci) && pci->ops && pci->ops->start_link) {
ret = pci->ops->start_link(pci);
if (ret)
goto err_free_msi;
}
> +{
> + int err;
> + u32 val;
> +
> + /* wait for linkup of phy link layer */
> + err = readl_relaxed_poll_timeout(pcie->ulreg_base + PCIE_UL_REG_V_PHY_ST_00, val,
> + (val & PCIE_UL_SMLH_LINK_UP), 1000, 10000);
> + if (err)
> + return err;
> +
> + /* wait for linkup of data link layer */
> + err = readl_relaxed_poll_timeout(pcie->ulreg_base + PCIE_UL_REG_V_PHY_ST_02, val,
> + (val & PCIE_UL_S_DETECT_ACT), 1000, 10000);
> + if (err)
> + return err;
> +
> + /* wait for LTSSM Status */
> + return readl_relaxed_poll_timeout(pcie->ulreg_base + PCIE_UL_REG_V_PHY_ST_02, val,
> + (val & PCIE_UL_S_L0), 1000, 10000);
> +}
> +
> +static int visconti_pcie_establish_link(struct pcie_port *pp)
> +{
> + int ret;
> + u32 val;
> + struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> + struct visconti_pcie *pcie = dev_get_drvdata(pci->dev);
Reorder these in the order of use (as you did below).
> + visconti_ulreg_writel(pcie, PCIE_UL_APP_LTSSM_ENABLE, PCIE_UL_REG_V_SII_GEN_CTRL_01);
> +
> + ret = visconti_pcie_check_link_status(pcie);
> + if (ret < 0) {
> + dev_info(pci->dev, "Link failure\n");
> + return ret;
> + }
> +
> + val = visconti_mpu_readl(pcie, PCIE_MPU_REG_MP_EN);
> + visconti_mpu_writel(pcie, val & ~MPU_MP_EN_DISABLE, PCIE_MPU_REG_MP_EN);
> +
> + visconti_ulreg_writel(pcie, PCIE_UL_S_INT_EVENT_MASK1_ALL, PCIE_UL_REG_S_INT_EVENT_MASK1);
> +
> + return 0;
> +}
> +
> +static int visconti_pcie_host_init(struct pcie_port *pp)
> +{
> + dw_pcie_setup_rc(pp);
> + return visconti_pcie_establish_link(pp);
This doesn't look like correct usage of the dwc infrastructure. No
other *_pcie_host_init() functions call dw_pcie_setup_rc(). It'd be
nice if the dwc infrastructure had a little more of a user's guide.
In the meantime, try to copy the structure and style of existing
drivers.
> +}
> +
> +static const struct dw_pcie_host_ops visconti_pcie_host_ops = {
> + .host_init = visconti_pcie_host_init,
> +};
> +
> +static u64 visconti_pcie_cpu_addr_fixup(struct dw_pcie *pci, u64 pci_addr)
> +{
> + return pci_addr - PCIE_BUS_OFFSET;
> +}
> +
> +static const struct dw_pcie_ops dw_pcie_ops = {
> + .cpu_addr_fixup = visconti_pcie_cpu_addr_fixup,
> +};
> +
> +static int visconti_get_resources(struct platform_device *pdev,
> + struct visconti_pcie *pcie)
> +{
> + struct device *dev = &pdev->dev;
> +
> + pcie->ulreg_base = devm_platform_ioremap_resource_byname(pdev, "ulreg");
> + if (IS_ERR(pcie->ulreg_base))
> + return PTR_ERR(pcie->ulreg_base);
> +
> + pcie->smu_base = devm_platform_ioremap_resource_byname(pdev, "smu");
> + if (IS_ERR(pcie->smu_base))
> + return PTR_ERR(pcie->smu_base);
> +
> + pcie->mpu_base = devm_platform_ioremap_resource_byname(pdev, "mpu");
> + if (IS_ERR(pcie->mpu_base))
> + return PTR_ERR(pcie->mpu_base);
> +
> + pcie->refclk = devm_clk_get(dev, "pcie_refclk");
> + if (IS_ERR(pcie->refclk)) {
> + dev_err(dev, "Failed to get refclk clock: %ld\n", PTR_ERR(pcie->refclk));
> + return PTR_ERR(pcie->refclk);
> + }
> +
> + pcie->sysclk = devm_clk_get(dev, "sysclk");
> + if (IS_ERR(pcie->sysclk)) {
> + dev_err(dev, "Failed to get sysclk clock: %ld\n", PTR_ERR(pcie->sysclk));
> + return PTR_ERR(pcie->sysclk);
> + }
> +
> + pcie->auxclk = devm_clk_get(dev, "auxclk");
> + if (IS_ERR(pcie->auxclk)) {
> + dev_err(dev, "Failed to get auxclk clock: %ld\n", PTR_ERR(pcie->auxclk));
> + return PTR_ERR(pcie->auxclk);
> + }
> +
> + return 0;
> +}
> +
> +static int visconti_device_turnon(struct visconti_pcie *pcie)
There are no other drivers with a *_turnon() function. Can you copy
a name used in other drivers for similar functionality? Maybe this
is similar to kirin_pcie_power_on(), meson_pcie_power_on(), etc?
> +{
> + int err;
> + u32 val;
> +
> + visconti_smu_writel(pcie, PISMU_CKON_PCIE_AUX_CLK | PISMU_CKON_PCIE_MSTR_ACLK,
> + PISMU_CKON_PCIE);
> + ndelay(250);
> +
> + visconti_smu_writel(pcie, PISMU_RSOFF_PCIE_ULREG_RST_N, PISMU_RSOFF_PCIE);
> +
> + visconti_ulreg_writel(pcie, PCIE_UL_REG_S_PCIE_MODE_RC, PCIE_UL_REG_S_PCIE_MODE);
> +
> + val = PCIE_UL_IOM_PCIE_PERSTN_I_EN | PCIE_UL_DIRECT_PERSTN_EN | PCIE_UL_DIRECT_PERSTN;
> + visconti_ulreg_writel(pcie, val, PCIE_UL_REG_S_PERSTN_CTRL);
> + udelay(100);
> +
> + val |= PCIE_UL_PERSTN_OUT;
> + visconti_ulreg_writel(pcie, val, PCIE_UL_REG_S_PERSTN_CTRL);
> + udelay(100);
> +
> + visconti_smu_writel(pcie, PISMU_RSOFF_PCIE_PWR_UP_RST_N, PISMU_RSOFF_PCIE);
> +
> + err = readl_relaxed_poll_timeout(pcie->ulreg_base + PCIE_UL_REG_S_PHY_INIT_03, val,
> + (val & PCIE_UL_PHY0_SRAM_INIT_DONE), 100, 1000);
> + if (err)
> + return err;
> +
> + visconti_ulreg_writel(pcie, PCIE_UL_PHY0_SRAM_EXT_LD_DONE, PCIE_UL_REG_S_PHY_INIT_02);
> +
> + return readl_relaxed_poll_timeout(pcie->ulreg_base + PCIE_UL_REG_S_SIG_MON, val,
> + (val & PCIE_UL_CORE_RST_N_MON), 100, 1000);
> +}
> +
> +static int visconti_add_pcie_port(struct visconti_pcie *pcie, struct platform_device *pdev)
> +{
> + struct dw_pcie *pci = &pcie->pci;
> + struct pcie_port *pp = &pci->pp;
> + struct device *dev = &pdev->dev;
> + int ret;
> +
> + pp->irq = platform_get_irq_byname(pdev, "intr");
> + if (pp->irq < 0) {
> + dev_err(dev, "interrupt intr is missing");
Make your error messages consistently capitalized (or consistently not
capitalized).
> + return pp->irq;
> + }
> +
> + pp->ops = &visconti_pcie_host_ops;
> +
> + pci->link_gen = of_pci_get_max_link_speed(pdev->dev.of_node);
> + if (pci->link_gen < 0 || pci->link_gen > 3) {
> + pci->link_gen = 3;
> + dev_dbg(dev, "Applied default link speed\n");
> + }
> +
> + dev_dbg(dev, "link speed Gen %d", pci->link_gen);
> +
> + ret = visconti_device_turnon(pcie);
> + if (ret)
> + goto error;
There's no cleanup, so just "return ret;" here and omit the "error:"
label.
> + ret = dw_pcie_host_init(pp);
> + if (ret)
> + dev_err(dev, "Failed to initialize host\n");
> +
> +error:
> + return ret;
> +}
> +
> +static int visconti_pcie_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct visconti_pcie *pcie;
> + struct pcie_port *pp;
> + struct dw_pcie *pci;
> + int ret;
> +
> + ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(36));
> + if (ret)
> + return ret;
> +
> + pcie = devm_kzalloc(dev, sizeof(*pcie), GFP_KERNEL);
> + if (!pcie)
> + return -ENOMEM;
> +
> + pci = &pcie->pci;
> + pp = &pci->pp;
> + pp->num_vectors = MAX_MSI_IRQS;
> +
> + pci->dev = dev;
> + pci->ops = &dw_pcie_ops;
> +
> + ret = visconti_get_resources(pdev, pcie);
> + if (ret)
> + return ret;
> +
> + platform_set_drvdata(pdev, pcie);
> +
> + return visconti_add_pcie_port(pcie, pdev);
> +}
> +
> +static const struct of_device_id visconti_pcie_match[] = {
> + { .compatible = "toshiba,visconti-pcie" },
> + {},
> +};
> +
> +static struct platform_driver visconti_pcie_driver = {
> + .probe = visconti_pcie_probe,
> + .driver = {
> + .name = "visconti-pcie",
> + .of_match_table = visconti_pcie_match,
> + .suppress_bind_attrs = true,
> + },
> +};
> +
> +builtin_platform_driver(visconti_pcie_driver);
> --
> 2.30.0.rc2
>
Hi,
Thanks for your review.
On Thu, Apr 29, 2021 at 06:10:40PM -0500, Bjorn Helgaas wrote:
> Please use something like this as the subject to match existing
> convention:
>
> PCI: visconti: Add Toshiba Visconti PCIe host controller driver
>
> (You can use "git log --oneline drivers/pci/controller/dwc" to learn
> this.)
OK, I will fix this.
>
> On Mon, Apr 19, 2021 at 03:35:12PM +0900, Nobuhiro Iwamatsu wrote:
> > Add support to PCIe RC controller on Toshiba Visconti ARM SoCs. PCIe controller is based of Synopsys
> > DesignWare PCIe core.
> >
> > This patch does not yet use the clock framework to control the clock. This will be replaced in the
> > future.
>
> Wrap this text to fit nicely in 75 columns.
I will fix this.
>
> > Signed-off-by: Yuji Ishikawa <[email protected]>
> > Signed-off-by: Nobuhiro Iwamatsu <[email protected]>
> > ---
> > drivers/pci/controller/dwc/Kconfig | 9 +
> > drivers/pci/controller/dwc/Makefile | 1 +
> > drivers/pci/controller/dwc/pcie-visconti.c | 333 +++++++++++++++++++++
> > 3 files changed, 343 insertions(+)
> > create mode 100644 drivers/pci/controller/dwc/pcie-visconti.c
> >
> > diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
> > index b9aaa84452c4..043c2c70d566 100644
> > --- a/drivers/pci/controller/dwc/Kconfig
> > +++ b/drivers/pci/controller/dwc/Kconfig
> > @@ -286,6 +286,15 @@ config PCIE_TEGRA194_EP
> > in order to enable device-specific features PCIE_TEGRA194_EP must be
> > selected. This uses the DesignWare core.
> >
> > +config PCIE_VISCONTI
>
> Consider using PCIE_VISCONTI_HOST in case you ever adapt this hardware
> to work in endpoint mode.
As you pointed out, this device also supports Endpoint mode. I will change to
PCIE_VISCONTI_HOST.
>
> > + bool "Toshiba VISCONTI PCIe controllers"
>
> s/VISCONTI/Visconti/ to match usage below.
OK.
>
> > + depends on ARCH_VISCONTI || COMPILE_TEST
> > + depends on PCI_MSI_IRQ_DOMAIN
> > + select PCIE_DW_HOST
> > + help
> > + Say Y here if you want PCIe controller support on Toshiba Visconti SoC.
> > + This driver supports TMPV77xx.
>
> What is "TMPV77xx"? A SoC? A reference platform?
This is a SoC. I will update description.
>
> > config PCIE_UNIPHIER
> > bool "Socionext UniPhier PCIe host controllers"
> > depends on ARCH_UNIPHIER || COMPILE_TEST
> > diff --git a/drivers/pci/controller/dwc/Makefile b/drivers/pci/controller/dwc/Makefile
> > index ba7c42f6df6f..46ac5d49dc75 100644
> > --- a/drivers/pci/controller/dwc/Makefile
> > +++ b/drivers/pci/controller/dwc/Makefile
> > @@ -20,6 +20,7 @@ obj-$(CONFIG_PCI_MESON) += pci-meson.o
> > obj-$(CONFIG_PCIE_TEGRA194) += pcie-tegra194.o
> > obj-$(CONFIG_PCIE_UNIPHIER) += pcie-uniphier.o
> > obj-$(CONFIG_PCIE_UNIPHIER_EP) += pcie-uniphier-ep.o
> > +obj-$(CONFIG_PCIE_VISCONTI) += pcie-visconti.o
> >
> > # The following drivers are for devices that use the generic ACPI
> > # pci_root.c driver but don't support standard ECAM config access.
> > diff --git a/drivers/pci/controller/dwc/pcie-visconti.c b/drivers/pci/controller/dwc/pcie-visconti.c
> > new file mode 100644
> > index 000000000000..a4739e7187f0
> > --- /dev/null
> > +++ b/drivers/pci/controller/dwc/pcie-visconti.c
> > @@ -0,0 +1,333 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * DWC PCIe RC driver for Toshiba Visconti ARM SoC
> > + *
> > + * Copyright (C) 2019, 2020 Toshiba Electronic Device & Storage Corporation
> > + * Copyright (C) 2020, TOSHIBA CORPORATION
> > + *
> > + * Nobuhiro Iwamatsu <[email protected]>
> > + *
>
> Remove spurious blank line.
>
OK, I will fix this.
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/delay.h>
> > +#include <linux/gpio.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/init.h>
> > +#include <linux/iopoll.h>
> > +#include <linux/kernel.h>
> > +#include <linux/of_platform.h>
> > +#include <linux/pci.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/resource.h>
> > +#include <linux/types.h>
> > +
> > +#include "pcie-designware.h"
> > +#include "../../pci.h"
> > +
> > +struct visconti_pcie {
> > + struct dw_pcie pci;
> > + void __iomem *ulreg_base;
> > + void __iomem *smu_base;
> > + void __iomem *mpu_base;
> > + struct clk *refclk;
> > + struct clk *sysclk;
> > + struct clk *auxclk;
> > +};
> > +
> > +#define PCIE_UL_REG_S_PCIE_MODE 0x00F4
> > +#define PCIE_UL_REG_S_PCIE_MODE_EP 0x00
> > +#define PCIE_UL_REG_S_PCIE_MODE_RC 0x04
> > +
> > +#define PCIE_UL_REG_S_PERSTN_CTRL 0x00F8
> > +#define PCIE_UL_IOM_PCIE_PERSTN_I_EN BIT(3)
> > +#define PCIE_UL_DIRECT_PERSTN_EN BIT(2)
> > +#define PCIE_UL_PERSTN_OUT BIT(1)
> > +#define PCIE_UL_DIRECT_PERSTN BIT(0)
> > +
> > +#define PCIE_UL_REG_S_PHY_INIT_02 0x0104
> > +#define PCIE_UL_PHY0_SRAM_EXT_LD_DONE BIT(0)
> > +
> > +#define PCIE_UL_REG_S_PHY_INIT_03 0x0108
> > +#define PCIE_UL_PHY0_SRAM_INIT_DONE BIT(0)
> > +
> > +#define PCIE_UL_REG_S_INT_EVENT_MASK1 0x0138
> > +#define PCIE_UL_CFG_PME_INT BIT(0)
> > +#define PCIE_UL_CFG_LINK_EQ_REQ_INT BIT(1)
> > +#define PCIE_UL_EDMA_INT0 BIT(2)
> > +#define PCIE_UL_EDMA_INT1 BIT(3)
> > +#define PCIE_UL_EDMA_INT2 BIT(4)
> > +#define PCIE_UL_EDMA_INT3 BIT(5)
> > +#define PCIE_UL_S_INT_EVENT_MASK1_ALL (PCIE_UL_CFG_PME_INT | PCIE_UL_CFG_LINK_EQ_REQ_INT | \
> > + PCIE_UL_EDMA_INT0 | PCIE_UL_EDMA_INT1 | \
> > + PCIE_UL_EDMA_INT2 | PCIE_UL_EDMA_INT3)
> > +
>
> Please wrap the code here and below so it fits nicely in 80 columns.
checkpatch.pl allows up to 100 characters, is this a PCI driver rule?
>
> > +#define PCIE_UL_REG_S_SB_MON 0x0198
> > +#define PCIE_UL_REG_S_SIG_MON 0x019C
> > +#define PCIE_UL_CORE_RST_N_MON BIT(0)
> > +
> > +#define PCIE_UL_REG_V_SII_DBG_00 0x0844
> > +#define PCIE_UL_REG_V_SII_GEN_CTRL_01 0x0860
> > +#define PCIE_UL_APP_LTSSM_ENABLE BIT(0)
> > +
> > +#define PCIE_UL_REG_V_PHY_ST_00 0x0864
> > +#define PCIE_UL_SMLH_LINK_UP BIT(0)
> > +
> > +#define PCIE_UL_REG_V_PHY_ST_02 0x0868
> > +#define PCIE_UL_S_DETECT_ACT 0x01
> > +#define PCIE_UL_S_L0 0x11
> > +
> > +#define PISMU_CKON_PCIE 0x0038
> > +#define PISMU_CKON_PCIE_AUX_CLK BIT(1)
> > +#define PISMU_CKON_PCIE_MSTR_ACLK BIT(0)
> > +
> > +#define PISMU_RSOFF_PCIE 0x0538
> > +#define PISMU_RSOFF_PCIE_ULREG_RST_N BIT(1)
> > +#define PISMU_RSOFF_PCIE_PWR_UP_RST_N BIT(0)
> > +
> > +#define PCIE_MPU_REG_MP_EN 0x0
> > +#define MPU_MP_EN_DISABLE BIT(0)
> > +
> > +#define PCIE_BUS_OFFSET 0x40000000
> > +
> > +/* Access registers in PCIe ulreg */
> > +static inline void visconti_ulreg_writel(struct visconti_pcie *pcie, u32 val, u32 reg)
> > +{
> > + writel_relaxed(val, pcie->ulreg_base + reg);
> > +}
> > +
> > +/* Access registers in PCIe smu */
> > +static inline void visconti_smu_writel(struct visconti_pcie *pcie, u32 val, u32 reg)
> > +{
> > + writel_relaxed(val, pcie->smu_base + reg);
> > +}
> > +
> > +/* Access registers in PCIe mpu */
> > +static inline void visconti_mpu_writel(struct visconti_pcie *pcie, u32 val, u32 reg)
> > +{
> > + writel_relaxed(val, pcie->mpu_base + reg);
> > +}
> > +
> > +static inline u32 visconti_mpu_readl(struct visconti_pcie *pcie, u32 reg)
>
> Don't bother with "inline" above and the signature will fit better in
> the line. The compiler will inline these even if you don't specify
> it.
OK, I will update this.
>
> > +{
> > + return readl_relaxed(pcie->mpu_base + reg);
> > +}
> > +
> > +static int visconti_pcie_check_link_status(struct visconti_pcie *pcie)
>
> "check_link_status" gives no hint about what the return value means.
> This looks like it might be similar to what other drivers do in
> *_pcie_link_up().
>
> Can you hook things up so this generic code in dw_pcie_host_init()
> works for you?
>
> if (!dw_pcie_link_up(pci) && pci->ops && pci->ops->start_link) {
> ret = pci->ops->start_link(pci);
> if (ret)
> goto err_free_msi;
> }
Thanks for your suggestion. I will investigate.
>
> > +{
> > + int err;
> > + u32 val;
> > +
> > + /* wait for linkup of phy link layer */
> > + err = readl_relaxed_poll_timeout(pcie->ulreg_base + PCIE_UL_REG_V_PHY_ST_00, val,
> > + (val & PCIE_UL_SMLH_LINK_UP), 1000, 10000);
> > + if (err)
> > + return err;
> > +
> > + /* wait for linkup of data link layer */
> > + err = readl_relaxed_poll_timeout(pcie->ulreg_base + PCIE_UL_REG_V_PHY_ST_02, val,
> > + (val & PCIE_UL_S_DETECT_ACT), 1000, 10000);
> > + if (err)
> > + return err;
> > +
> > + /* wait for LTSSM Status */
> > + return readl_relaxed_poll_timeout(pcie->ulreg_base + PCIE_UL_REG_V_PHY_ST_02, val,
> > + (val & PCIE_UL_S_L0), 1000, 10000);
> > +}
> > +
> > +static int visconti_pcie_establish_link(struct pcie_port *pp)
> > +{
> > + int ret;
> > + u32 val;
> > + struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > + struct visconti_pcie *pcie = dev_get_drvdata(pci->dev);
>
> Reorder these in the order of use (as you did below).
Ok, I will fix.
>
> > + visconti_ulreg_writel(pcie, PCIE_UL_APP_LTSSM_ENABLE, PCIE_UL_REG_V_SII_GEN_CTRL_01);
> > +
> > + ret = visconti_pcie_check_link_status(pcie);
> > + if (ret < 0) {
> > + dev_info(pci->dev, "Link failure\n");
> > + return ret;
> > + }
> > +
> > + val = visconti_mpu_readl(pcie, PCIE_MPU_REG_MP_EN);
> > + visconti_mpu_writel(pcie, val & ~MPU_MP_EN_DISABLE, PCIE_MPU_REG_MP_EN);
> > +
> > + visconti_ulreg_writel(pcie, PCIE_UL_S_INT_EVENT_MASK1_ALL, PCIE_UL_REG_S_INT_EVENT_MASK1);
> > +
> > + return 0;
> > +}
> > +
> > +static int visconti_pcie_host_init(struct pcie_port *pp)
> > +{
> > + dw_pcie_setup_rc(pp);
> > + return visconti_pcie_establish_link(pp);
>
> This doesn't look like correct usage of the dwc infrastructure. No
> other *_pcie_host_init() functions call dw_pcie_setup_rc(). It'd be
> nice if the dwc infrastructure had a little more of a user's guide.
> In the meantime, try to copy the structure and style of existing
> drivers.
>
OK, I will check using dw_pcie_setup_rc().
> > +}
> > +
> > +static const struct dw_pcie_host_ops visconti_pcie_host_ops = {
> > + .host_init = visconti_pcie_host_init,
> > +};
> > +
> > +static u64 visconti_pcie_cpu_addr_fixup(struct dw_pcie *pci, u64 pci_addr)
> > +{
> > + return pci_addr - PCIE_BUS_OFFSET;
> > +}
> > +
> > +static const struct dw_pcie_ops dw_pcie_ops = {
> > + .cpu_addr_fixup = visconti_pcie_cpu_addr_fixup,
> > +};
> > +
> > +static int visconti_get_resources(struct platform_device *pdev,
> > + struct visconti_pcie *pcie)
> > +{
> > + struct device *dev = &pdev->dev;
> > +
> > + pcie->ulreg_base = devm_platform_ioremap_resource_byname(pdev, "ulreg");
> > + if (IS_ERR(pcie->ulreg_base))
> > + return PTR_ERR(pcie->ulreg_base);
> > +
> > + pcie->smu_base = devm_platform_ioremap_resource_byname(pdev, "smu");
> > + if (IS_ERR(pcie->smu_base))
> > + return PTR_ERR(pcie->smu_base);
> > +
> > + pcie->mpu_base = devm_platform_ioremap_resource_byname(pdev, "mpu");
> > + if (IS_ERR(pcie->mpu_base))
> > + return PTR_ERR(pcie->mpu_base);
> > +
> > + pcie->refclk = devm_clk_get(dev, "pcie_refclk");
> > + if (IS_ERR(pcie->refclk)) {
> > + dev_err(dev, "Failed to get refclk clock: %ld\n", PTR_ERR(pcie->refclk));
> > + return PTR_ERR(pcie->refclk);
> > + }
> > +
> > + pcie->sysclk = devm_clk_get(dev, "sysclk");
> > + if (IS_ERR(pcie->sysclk)) {
> > + dev_err(dev, "Failed to get sysclk clock: %ld\n", PTR_ERR(pcie->sysclk));
> > + return PTR_ERR(pcie->sysclk);
> > + }
> > +
> > + pcie->auxclk = devm_clk_get(dev, "auxclk");
> > + if (IS_ERR(pcie->auxclk)) {
> > + dev_err(dev, "Failed to get auxclk clock: %ld\n", PTR_ERR(pcie->auxclk));
> > + return PTR_ERR(pcie->auxclk);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int visconti_device_turnon(struct visconti_pcie *pcie)
>
> There are no other drivers with a *_turnon() function. Can you copy
> a name used in other drivers for similar functionality? Maybe this
> is similar to kirin_pcie_power_on(), meson_pcie_power_on(), etc?
>
Yes, It provides the same functionality as kirin_pcie_power_on() and so
on. I will change the function name.
> > +{
> > + int err;
> > + u32 val;
> > +
> > + visconti_smu_writel(pcie, PISMU_CKON_PCIE_AUX_CLK | PISMU_CKON_PCIE_MSTR_ACLK,
> > + PISMU_CKON_PCIE);
> > + ndelay(250);
> > +
> > + visconti_smu_writel(pcie, PISMU_RSOFF_PCIE_ULREG_RST_N, PISMU_RSOFF_PCIE);
> > +
> > + visconti_ulreg_writel(pcie, PCIE_UL_REG_S_PCIE_MODE_RC, PCIE_UL_REG_S_PCIE_MODE);
> > +
> > + val = PCIE_UL_IOM_PCIE_PERSTN_I_EN | PCIE_UL_DIRECT_PERSTN_EN | PCIE_UL_DIRECT_PERSTN;
> > + visconti_ulreg_writel(pcie, val, PCIE_UL_REG_S_PERSTN_CTRL);
> > + udelay(100);
> > +
> > + val |= PCIE_UL_PERSTN_OUT;
> > + visconti_ulreg_writel(pcie, val, PCIE_UL_REG_S_PERSTN_CTRL);
> > + udelay(100);
> > +
> > + visconti_smu_writel(pcie, PISMU_RSOFF_PCIE_PWR_UP_RST_N, PISMU_RSOFF_PCIE);
> > +
> > + err = readl_relaxed_poll_timeout(pcie->ulreg_base + PCIE_UL_REG_S_PHY_INIT_03, val,
> > + (val & PCIE_UL_PHY0_SRAM_INIT_DONE), 100, 1000);
> > + if (err)
> > + return err;
> > +
> > + visconti_ulreg_writel(pcie, PCIE_UL_PHY0_SRAM_EXT_LD_DONE, PCIE_UL_REG_S_PHY_INIT_02);
> > +
> > + return readl_relaxed_poll_timeout(pcie->ulreg_base + PCIE_UL_REG_S_SIG_MON, val,
> > + (val & PCIE_UL_CORE_RST_N_MON), 100, 1000);
> > +}
> > +
> > +static int visconti_add_pcie_port(struct visconti_pcie *pcie, struct platform_device *pdev)
> > +{
> > + struct dw_pcie *pci = &pcie->pci;
> > + struct pcie_port *pp = &pci->pp;
> > + struct device *dev = &pdev->dev;
> > + int ret;
> > +
> > + pp->irq = platform_get_irq_byname(pdev, "intr");
> > + if (pp->irq < 0) {
> > + dev_err(dev, "interrupt intr is missing");
>
> Make your error messages consistently capitalized (or consistently not
> capitalized).
Sorry, I didn't understand this point correctly.
Does this mean capitalize the first letter of the message?
>
> > + return pp->irq;
> > + }
> > +
> > + pp->ops = &visconti_pcie_host_ops;
> > +
> > + pci->link_gen = of_pci_get_max_link_speed(pdev->dev.of_node);
> > + if (pci->link_gen < 0 || pci->link_gen > 3) {
> > + pci->link_gen = 3;
> > + dev_dbg(dev, "Applied default link speed\n");
> > + }
> > +
> > + dev_dbg(dev, "link speed Gen %d", pci->link_gen);
> > +
> > + ret = visconti_device_turnon(pcie);
> > + if (ret)
> > + goto error;
>
> There's no cleanup, so just "return ret;" here and omit the "error:"
> label.
Indeed. I will update this.
>
> > + ret = dw_pcie_host_init(pp);
> > + if (ret)
> > + dev_err(dev, "Failed to initialize host\n");
> > +
> > +error:
> > + return ret;
> > +}
> > +
> > +static int visconti_pcie_probe(struct platform_device *pdev)
> > +{
> > + struct device *dev = &pdev->dev;
> > + struct visconti_pcie *pcie;
> > + struct pcie_port *pp;
> > + struct dw_pcie *pci;
> > + int ret;
> > +
> > + ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(36));
> > + if (ret)
> > + return ret;
> > +
> > + pcie = devm_kzalloc(dev, sizeof(*pcie), GFP_KERNEL);
> > + if (!pcie)
> > + return -ENOMEM;
> > +
> > + pci = &pcie->pci;
> > + pp = &pci->pp;
> > + pp->num_vectors = MAX_MSI_IRQS;
> > +
> > + pci->dev = dev;
> > + pci->ops = &dw_pcie_ops;
> > +
> > + ret = visconti_get_resources(pdev, pcie);
> > + if (ret)
> > + return ret;
> > +
> > + platform_set_drvdata(pdev, pcie);
> > +
> > + return visconti_add_pcie_port(pcie, pdev);
> > +}
> > +
> > +static const struct of_device_id visconti_pcie_match[] = {
> > + { .compatible = "toshiba,visconti-pcie" },
> > + {},
> > +};
> > +
> > +static struct platform_driver visconti_pcie_driver = {
> > + .probe = visconti_pcie_probe,
> > + .driver = {
> > + .name = "visconti-pcie",
> > + .of_match_table = visconti_pcie_match,
> > + .suppress_bind_attrs = true,
> > + },
> > +};
> > +
> > +builtin_platform_driver(visconti_pcie_driver);
> > --
> > 2.30.0.rc2
> >
>
Best regards,
Nobuhiro
On Mon, May 10, 2021 at 04:47:36PM +0900, Nobuhiro Iwamatsu wrote:
> On Thu, Apr 29, 2021 at 06:10:40PM -0500, Bjorn Helgaas wrote:
> > > +#define PCIE_UL_EDMA_INT3 BIT(5)
> > > +#define PCIE_UL_S_INT_EVENT_MASK1_ALL (PCIE_UL_CFG_PME_INT | PCIE_UL_CFG_LINK_EQ_REQ_INT | \
> > > + PCIE_UL_EDMA_INT0 | PCIE_UL_EDMA_INT1 | \
> > > + PCIE_UL_EDMA_INT2 | PCIE_UL_EDMA_INT3)
> > > +
> >
> > Please wrap the code here and below so it fits nicely in 80 columns.
>
> checkpatch.pl allows up to 100 characters, is this a PCI driver rule?
The general rule is "match what's around you." So I guess that makes
it a drivers/pci/ rule.
> > > + pp->irq = platform_get_irq_byname(pdev, "intr");
> > > + if (pp->irq < 0) {
> > > + dev_err(dev, "interrupt intr is missing");
> >
> > Make your error messages consistently capitalized (or consistently not
> > capitalized).
>
> Sorry, I didn't understand this point correctly.
> Does this mean capitalize the first letter of the message?
Some of your messages are capitalized and others are not:
dev_info(pci->dev, "Link failure\n"
dev_err(dev, "Failed to get refclk clock: %ld\n"
dev_err(dev, "Failed to get sysclk clock: %ld\n"
dev_err(dev, "Failed to get auxclk clock: %ld\n"
dev_err(dev, "interrupt intr is missing"
dev_dbg(dev, "Applied default link speed\n"
dev_dbg(dev, "link speed Gen %d"
dev_err(dev, "Failed to initialize host\n"
I don't really care whether they're all capitalized or none are
capitalized, but they should all be the same. Otherwise it just looks
sloppy.
Bjorn