PCI: Add new Unisoc PCIe driver
Subsystems (4)
v6 cover PCI: Add new Unisoc PCIe driver
From: Hongtao Wu <[email protected]>
This series adds PCIe controller driver for Unisoc SoCs. This controller is based on DesignWare PCIe IP.
Changes from v1:
1) Test this patch on top of Rob Herring's 40 part series of DWC clean-ups:
https://lore.kernel.org/linux-pci/[email protected]/
2) Delete empty function
3) Document property "sprd,pcie-poweron-syscons" and 'sprd,pcie-poweroff-syscons'
4) Delete runtime suspend/resume function
5) Add COMPILE_TEST which CONFIG_PCIE_SPRD depends on
Changes from v2:
1) Change RC mode to host mode in drivers/pci/controller/dwc/Kconfig
2) Change Signed-off-by from Billows Wu to Hongtao Wu
Changes from v3:
1) Split the property 'sprd,pcie-poweron-syscons' and 'sprd,pcie-poweroff-syscons' into reset, power domains, phy and so on.
2) Delete the function to get resource 'msi' and 'dbi' which were parsed by the DW core.
3) Delete the function 'sprd_pcie_host_init', because the DW core has done it.
Changes from v4:
1) Install 'yamllint' and upgrade dt-schema in order to solve the yamllint and dtschema/dtc warnings/errors:
Changes from v5:
1) Change "GPL v2" to "GPL".
2) Remove exit_p wrapper which used to define remove callback.
Hongtao Wu (2):
dt-bindings: PCI: sprd: Document Unisoc PCIe RC host controller
PCI: sprd: Add support for Unisoc SoCs' PCIe controller
.../devicetree/bindings/pci/sprd-pcie.yaml | 93 +++++++ drivers/pci/controller/dwc/Kconfig | 12 + drivers/pci/controller/dwc/Makefile | 1 + drivers/pci/controller/dwc/pcie-sprd.c | 293 +++++++++++++++++++++ 4 files changed, 399 insertions(+) create mode 100644 Documentation/devicetree/bindings/pci/sprd-pcie.yaml create mode 100644 drivers/pci/controller/dwc/pcie-sprd.c
--
2.7.4
PATCH v6 1/2 dt-bindings: PCI: sprd: Document Unisoc PCIe RC host controller
From: Hongtao Wu <[email protected]>
This series adds PCIe bindings for Unisoc SoCs.
This controller is based on DesignWare PCIe IP.
--- .../devicetree/bindings/pci/sprd-pcie.yaml | 93 ++++++++++++++++++++++ 1 file changed, 93 insertions(+) create mode 100644 Documentation/devicetree/bindings/pci/sprd-pcie.yaml
Documentation/devicetree/bindings/pci/sprd-pcie.yaml new file +93 -0
@@ -0,0 +1,93 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/pci/sprd-pcie.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: SoC PCIe Host Controller Device Tree Bindings + +maintainers: + - Hongtao Wu <[email protected]> + +allOf: + - $ref: /schemas/pci/pci-bus.yaml# + +properties: + compatible: + items: + - const: sprd,ums9520-pcie + + reg: + minItems: 2 + items: + - description: Controller control and status registers. + - description: PCIe configuration registers. + + reg-names: + items: + - const: dbi + - const: config + + ranges: + maxItems: 2 + + num-lanes: + maximum: 1 + description: Number of lanes to use for this port. + + interrupts: + minItems: 1 + description: Builtin MSI controller and PCIe host controller. + + interrupt-names: + items: + - const: msi + + sprd,regmap-aon: + $ref: '/schemas/types.yaml#/definitions/phandle' + description: + Phandle to the AON system controller node (to access the + AON_ACCESS_PCIE_EN register on ums9520). + sprd,regmap-pmu: + $ref: '/schemas/types.yaml#/definitions/phandle' + description: + Phandle to the PMU system controller node (to access the PERST_N_ASSERT + register on ums9520). + +required: + - compatible + - reg + - reg-names + - num-lanes + - ranges + - interrupts + - interrupt-names + +additionalProperties: true + +examples: + - | + #include <dt-bindings/interrupt-controller/arm-gic.h> + + ipa { + #address-cells = <2>; + #size-cells = <2>; + + pcie0: pcie@2b100000 { + compatible = "sprd,ums9520-pcie"; + reg = <0x0 0x2b100000 0x0 0x2000>, + <0x2 0x00000000 0x0 0x2000>; + reg-names = "dbi", "config"; + #address-cells = <3>; + #size-cells = <2>; + device_type = "pci"; + ranges = <0x01000000 0x0 0x00000000 0x2 0x00002000 0x0 0x00010000>, + <0x03000000 0x0 0x10000000 0x2 0x10000000 0x1 0xefffffff>; + num-lanes = <1>; + interrupts = <GIC_SPI 153 IRQ_TYPE_LEVEL_HIGH>; + interrupt-names = "msi"; + + sprd,regmap-aon = <&aon_regs>; + sprd,regmap-pmu = <&pmu_regs>; + }; + };
--
2.7.4
PATCH v6 2/2 PCI: sprd: Add support for Unisoc SoCs' PCIe controller
In reply to Hongtao WuFrom: Hongtao Wu <[email protected]>
This series adds PCIe controller driver for Unisoc SoCs. This controller is based on DesignWare PCIe IP.
--- drivers/pci/controller/dwc/Kconfig | 12 ++ drivers/pci/controller/dwc/Makefile | 1 + drivers/pci/controller/dwc/pcie-sprd.c | 293 +++++++++++++++++++++++++++++++++ 3 files changed, 306 insertions(+) create mode 100644 drivers/pci/controller/dwc/pcie-sprd.c
drivers/pci/controller/dwc/Kconfig +12 -0
@@ -318,4 +318,16 @@ config PCIE_AL required only for DT-based platforms. ACPI platforms with the Annapurna Labs PCIe controller don't need to enable this. +config PCIE_SPRD + tristate "Unisoc PCIe controller - Host Mode" + depends on ARCH_SPRD || COMPILE_TEST + depends on PCI_MSI_IRQ_DOMAIN + select PCIE_DW_HOST + help + Unisoc PCIe controller uses the DesignWare core. It can be configured + as an Endpoint (EP) or a Root complex (RC). In order to enable host + mode (the controller works as RC), PCIE_SPRD must be selected. + Say Y or M here if you want to PCIe RC controller support on Unisoc + SoCs. + endmenu
drivers/pci/controller/dwc/Makefile +1 -0
@@ -20,6 +20,7 @@ obj-$(CONFIG_PCI_MESON) += pci-meson.o obj-$(CONFIG_PCIE_TEGRA194) += pcie-tegra194.o obj-$(CONFIG_PCIE_UNIPHIER) += pcie-uniphier.o obj-$(CONFIG_PCIE_UNIPHIER_EP) += pcie-uniphier-ep.o +obj-$(CONFIG_PCIE_SPRD) += pcie-sprd.o # The following drivers are for devices that use the generic ACPI # pci_root.c driver but don't support standard ECAM config access.
drivers/pci/controller/dwc/pcie-sprd.c new file +293 -0
293 lines not on this page · raw message
--
2.7.4
Re: [PATCH v6 2/2] PCI: sprd: Add support for Unisoc SoCs' PCIe controller
Quoted 108 lines · Hongtao Wu
From: Hongtao Wu <[email protected]>
This series adds PCIe controller driver for Unisoc SoCs. This controller is based on DesignWare PCIe IP.
Signed-off-by Hongtao Wu <[email protected]>--- drivers/pci/controller/dwc/Kconfig | 12 ++ drivers/pci/controller/dwc/Makefile | 1 + drivers/pci/controller/dwc/pcie-sprd.c | 293 +++++++++++++++++++++++++++++++++ 3 files changed, 306 insertions(+) create mode 100644 drivers/pci/controller/dwc/pcie-sprd.cdrivers/pci/controller/dwc/Kconfig +12
-0@@ -318,4 +318,16 @@ config PCIE_AL required only for DT-based platforms. ACPI platforms with the Annapurna Labs PCIe controller don't need to enable this. +config PCIE_SPRD + tristate "Unisoc PCIe controller - Host Mode" + depends on ARCH_SPRD || COMPILE_TEST + depends on PCI_MSI_IRQ_DOMAIN + select PCIE_DW_HOST + help + Unisoc PCIe controller uses the DesignWare core. It can be configured + as an Endpoint (EP) or a Root complex (RC). In order to enable host + mode (the controller works as RC), PCIE_SPRD must be selected. + Say Y or M here if you want to PCIe RC controller support on Unisoc + SoCs. + endmenudrivers/pci/controller/dwc/Makefile +1
-0@@ -20,6 +20,7 @@ obj-$(CONFIG_PCI_MESON) += pci-meson.o obj-$(CONFIG_PCIE_TEGRA194) += pcie-tegra194.o obj-$(CONFIG_PCIE_UNIPHIER) += pcie-uniphier.o obj-$(CONFIG_PCIE_UNIPHIER_EP) += pcie-uniphier-ep.o +obj-$(CONFIG_PCIE_SPRD) += pcie-sprd.o # The following drivers are for devices that use the generic ACPI # pci_root.c driver but don't support standard ECAM config access.drivers/pci/controller/dwc/pcie-sprd.c new file +56
-0@@ -0,0 +1,293 @@ +// SPDX-License-Identifier: GPL-2.0 +/* +* PCIe host controller driver for Unisoc SoCs +* +* Copyright (C) 2020-2021 Unisoc, Inc. +* +* Author: Hongtao Wu <[email protected]> +*/ + +#include <linux/delay.h> +#include <linux/gpio/consumer.h> +#include <linux/interrupt.h> +#include <linux/mfd/syscon.h> +#include <linux/module.h> +#include <linux/of_device.h> +#include <linux/of_irq.h> +#include <linux/platform_device.h> +#include <linux/property.h> +#include <linux/regmap.h> + +#include "pcie-designware.h" + +/* aon apb syscon */ +#define IPA_ACCESS_CFG 0xcd8 +#define AON_ACCESS_PCIE_EN BIT(1) + +/* pmu apb syscon */ +#define SNPS_PCIE3_SLP_CTRL 0xac +#define PERST_N_ASSERT BIT(1) +#define PERST_N_AUTO_EN BIT(0) +#define PD_PCIE_CFG_0 0x3e8 +#define PCIE_FORCE_SHUTDOWN BIT(25) + +#define PCIE_SS_REG_BASE 0xE00 +#define APB_CLKFREQ_TIMEOUT 0x4 +#define BUSERR_EN BIT(12) +#define APB_TIMER_DIS BIT(10) +#define APB_TIMER_LIMIT GENMASK(31, 16) + +#define PE0_GEN_CTRL_3 0x58 +#define LTSSM_EN BIT(0) + +struct sprd_pcie_soc_data { +u32 syscon_offset; +}; + +static const struct sprd_pcie_soc_data ums9520_syscon_data = { + .syscon_offset =0x1000 ,/* The offset of set/clear register */ +}; + +struct sprd_pcie { +u32 syscon_offset; +struct device *dev; +struct dw_pcie *pci; +struct regmap *aon_map; +struct regmap *pmu_map;
drivers/pci/controller/dwc/pcie-sprd.c line 66
Quoted 10 lines
drivers/pci/controller/dwc/pcie-sprd.c new file +10
-0+const struct sprd_pcie_soc_data *socdata; +}; + +enum sprd_pcie_syscon_type { + normal_syscon,/* it's not a set/clear register */ + set_syscon,/* set a set/clear register */ + clr_syscon,/* clear a set/clear register */ +}; + +static void sprd_pcie_buserr_enable (struct dw_pcie *pci)
I think you should add a comment to explain what this function enables - it is unclear.
Quoted 70 lines
drivers/pci/controller/dwc/pcie-sprd.c new file +70
-0+{ +u32 val; + + val =dw_pcie_readl_dbi (pci,PCIE_SS_REG_BASE +APB_CLKFREQ_TIMEOUT ); + val &= ~APB_TIMER_DIS ; + val |=BUSERR_EN ; + val |=APB_TIMER_LIMIT & (0x1f4 <<16 ); +dw_pcie_writel_dbi (pci,PCIE_SS_REG_BASE +APB_CLKFREQ_TIMEOUT , val); +} + +static void sprd_pcie_ltssm_enable (struct dw_pcie *pci,bool enable) +{ +u32 val; + + val =dw_pcie_readl_dbi (pci,PCIE_SS_REG_BASE +PE0_GEN_CTRL_3 ); +if (enable) +dw_pcie_writel_dbi (pci,PCIE_SS_REG_BASE +PE0_GEN_CTRL_3 , + val |LTSSM_EN ); +else +dw_pcie_writel_dbi (pci,PCIE_SS_REG_BASE +PE0_GEN_CTRL_3 , + val & ~LTSSM_EN ); +} + +static int sprd_pcie_syscon_set (struct sprd_pcie *ctrl,struct regmap *map, +u32 reg,u32 mask,u32 val, +enum sprd_pcie_syscon_type type) +{ +int ret =0 ; +u32 read_val; +u32 offset = ctrl->syscon_offset; +struct device *dev = ctrl->pci->dev; + +/* +* Each set/clear register has three registers: +* reg: base register +* reg + offset: set register +* reg + offset * 2: clear register +*/ +switch (type) { +case normal_syscon: + ret =regmap_read (map, reg, &read_val); +if (ret) { +dev_err (dev,"failed to read register 0x%x\n" , reg); +return ret; + } + read_val &= ~mask; + read_val |= (val & mask); + ret =regmap_write (map, reg, read_val); +break ; +case set_syscon: + reg = reg + offset; + ret =regmap_write (map, reg, val); +break ; +case clr_syscon: + reg = reg + offset *2 ; + ret =regmap_write (map, reg, val); +break ; +default : +break ; + } + +if (ret) +dev_err (dev,"failed to write register 0x%x\n" , reg); + +return ret; +} + +static int sprd_pcie_perst_assert (struct sprd_pcie *ctrl) +{ + return sprd_pcie_syscon_set(ctrl, ctrl->pmu_map, SNPS_PCIE3_SLP_CTRL,
drivers/pci/controller/dwc/pcie-sprd.c line 146
Quoted 10 lines
drivers/pci/controller/dwc/pcie-sprd.c new file +10
-0+ PERST_N_ASSERT, PERST_N_ASSERT, set_syscon); +} + +static int sprd_pcie_perst_deassert (struct sprd_pcie *ctrl) +{ +int ret; + + ret =sprd_pcie_syscon_set (ctrl, ctrl->pmu_map,SNPS_PCIE3_SLP_CTRL , +PERST_N_ASSERT ,0 , clr_syscon); +usleep_range (2000 ,3000 );
This delay seems arbitrary. The PCI specifications require a 100ms after issuing PERST before link training is enabled.
Have a look at e.g. drivers/pci/controller/pci-aardvark.c
Quoted 93 lines
drivers/pci/controller/dwc/pcie-sprd.c new file +93
-0+ +return ret; +} + +static int sprd_pcie_power_on (struct platform_device *pdev) +{ +int ret; +struct sprd_pcie *ctrl =platform_get_drvdata (pdev); +struct dw_pcie *pci = ctrl->pci; + + ret =sprd_pcie_syscon_set (ctrl, ctrl->aon_map,PD_PCIE_CFG_0 , +PCIE_FORCE_SHUTDOWN ,0 , clr_syscon); +if (ret) +return ret; + + ret =sprd_pcie_syscon_set (ctrl, ctrl->aon_map,IPA_ACCESS_CFG , +AON_ACCESS_PCIE_EN ,AON_ACCESS_PCIE_EN , + set_syscon); +if (ret) +return ret; + + ret =sprd_pcie_perst_deassert (ctrl); +if (ret) +return ret; + +sprd_pcie_buserr_enable (pci); +sprd_pcie_ltssm_enable (pci, true); + +return ret; +} + +static int sprd_pcie_power_off (struct platform_device *pdev) +{ +struct sprd_pcie *ctrl =platform_get_drvdata (pdev); +struct dw_pcie *pci = ctrl->pci; + +sprd_pcie_ltssm_enable (pci, false); + +sprd_pcie_perst_assert (ctrl); +sprd_pcie_syscon_set (ctrl, ctrl->aon_map,PD_PCIE_CFG_0 , +PCIE_FORCE_SHUTDOWN ,PCIE_FORCE_SHUTDOWN , + set_syscon); +sprd_pcie_syscon_set (ctrl, ctrl->aon_map,IPA_ACCESS_CFG , +AON_ACCESS_PCIE_EN ,0 , clr_syscon); + +return 0 ; +} + +static int sprd_add_pcie_port (struct platform_device *pdev) +{ +struct sprd_pcie *ctrl =platform_get_drvdata (pdev); +struct dw_pcie *pci = ctrl->pci; +struct pcie_port *pp = &pci->pp; + +return dw_pcie_host_init (pp); +} + +static int sprd_pcie_probe (struct platform_device *pdev) +{ +struct device *dev = &pdev->dev; +struct sprd_pcie *ctrl; +struct dw_pcie *pci; +int ret; + + ctrl =devm_kzalloc (dev,sizeof (*ctrl),GFP_KERNEL ); +if (!ctrl) +return -ENOMEM ; + + ctrl->socdata = + (struct sprd_pcie_soc_data *)of_device_get_match_data (dev); +if (!ctrl->socdata) { +dev_warn (dev, +"using the default set/clear register offset address" ); + ctrl->syscon_offset =0x1000 ; + } + ctrl->syscon_offset = ctrl->socdata->syscon_offset; + + ctrl->aon_map =syscon_regmap_lookup_by_phandle (dev->of_node, +"sprd, regmap-aon" ); +if (IS_ERR (ctrl->aon_map)) { +dev_err (dev,"failed to get syscon regmap aon\n" ); + ret =PTR_ERR (ctrl->aon_map); + goto err; + } + + ctrl->pmu_map =syscon_regmap_lookup_by_phandle (dev->of_node, +"sprd, regmap-pmu" ); +if (IS_ERR (ctrl->pmu_map)) { +dev_err (dev,"failed to get syscon regmap pmu\n" ); + ret =PTR_ERR (ctrl->pmu_map); + goto err; + } +
drivers/pci/controller/dwc/pcie-sprd.c line 249
Quoted 10 lines
drivers/pci/controller/dwc/pcie-sprd.c new file +10
-0+ pci = ctrl->pci; + pci->dev = dev; + +platform_set_drvdata (pdev, ctrl); + + ret =sprd_pcie_power_on (pdev); +if (ret <0 ) { +dev_err (dev,"failed to power on, return %d\n" , + ret); + goto err_power_off;
This looks a bit weird but I believe that's just a naming convention.
It looks like if we *fail* to power the controller on we power it off, which does not make much sense.
I guess the goal in err_power_off is to clean-up what was done in sprd_pcie_power_on() - more below.
drivers/pci/controller/dwc/pcie-sprd.c line 255
Quoted 6 lines
drivers/pci/controller/dwc/pcie-sprd.c new file +6
-0+ } + + ret =sprd_add_pcie_port (pdev); +if (ret) { +dev_warn (dev,"failed to initialize RC controller\n" ); +return ret;
If we go by the same logic, we shouldn't return but rather
goto err_power_off;
on error here. Am I missing something ?
Lorenzo
Quoted 41 lines
drivers/pci/controller/dwc/pcie-sprd.c new file +38
-1+ } + +return 0 ; + +err_power_off: +sprd_pcie_power_off (pdev); +err: +return ret; +} + +static int sprd_pcie_remove (struct platform_device *pdev) +{ +sprd_pcie_power_off (pdev); + +return 0 ; +} + +static const struct of_device_id sprd_pcie_of_match[] = { + { + .compatible ="sprd,ums9520-pcie" , + .data = &ums9520_syscon_data, + }, + {}, +}; + +static struct platform_driver sprd_pcie_driver = { + .probe = sprd_pcie_probe, + .remove = sprd_pcie_remove, + .driver = { + .name ="sprd-pcie" , + .of_match_table = sprd_pcie_of_match, + }, +}; + +module_platform_driver (sprd_pcie_driver); + +MODULE_DESCRIPTION ("Unisoc PCIe host controller driver" ); +MODULE_LICENSE ("GPL" ); --2.7.4
Chunyan Zhang
reviewerRe: [PATCH v6 2/2] PCI: sprd: Add support for Unisoc SoCs' PCIe controller
Quoted 115 lines · Lorenzo Pieralisi
Quoted 113 lines · Hongtao Wu
From: Hongtao Wu <[email protected]>
This series adds PCIe controller driver for Unisoc SoCs. This controller is based on DesignWare PCIe IP.
Signed-off-by Hongtao Wu <[email protected]>--- drivers/pci/controller/dwc/Kconfig | 12 ++ drivers/pci/controller/dwc/Makefile | 1 + drivers/pci/controller/dwc/pcie-sprd.c | 293 +++++++++++++++++++++++++++++++++ 3 files changed, 306 insertions(+) create mode 100644 drivers/pci/controller/dwc/pcie-sprd.cdrivers/pci/controller/dwc/Kconfig +12
-0@@ -318,4 +318,16 @@ config PCIE_AL required only for DT-based platforms. ACPI platforms with the Annapurna Labs PCIe controller don't need to enable this. +config PCIE_SPRD + tristate "Unisoc PCIe controller - Host Mode" + depends on ARCH_SPRD || COMPILE_TEST + depends on PCI_MSI_IRQ_DOMAIN + select PCIE_DW_HOST + help + Unisoc PCIe controller uses the DesignWare core. It can be configured + as an Endpoint (EP) or a Root complex (RC). In order to enable host + mode (the controller works as RC), PCIE_SPRD must be selected. + Say Y or M here if you want to PCIe RC controller support on Unisoc + SoCs. + endmenudrivers/pci/controller/dwc/Makefile +1
-0@@ -20,6 +20,7 @@ obj-$(CONFIG_PCI_MESON) += pci-meson.o obj-$(CONFIG_PCIE_TEGRA194) += pcie-tegra194.o obj-$(CONFIG_PCIE_UNIPHIER) += pcie-uniphier.o obj-$(CONFIG_PCIE_UNIPHIER_EP) += pcie-uniphier-ep.o +obj-$(CONFIG_PCIE_SPRD) += pcie-sprd.o # The following drivers are for devices that use the generic ACPI # pci_root.c driver but don't support standard ECAM config access.drivers/pci/controller/dwc/pcie-sprd.c new file +61
-0@@ -0,0 +1,293 @@ +// SPDX-License-Identifier: GPL-2.0 +/* +* PCIe host controller driver for Unisoc SoCs +* +* Copyright (C) 2020-2021 Unisoc, Inc. +* +* Author: Hongtao Wu <[email protected]> +*/ + +#include <linux/delay.h> +#include <linux/gpio/consumer.h> +#include <linux/interrupt.h> +#include <linux/mfd/syscon.h> +#include <linux/module.h> +#include <linux/of_device.h> +#include <linux/of_irq.h> +#include <linux/platform_device.h> +#include <linux/property.h> +#include <linux/regmap.h> + +#include "pcie-designware.h" + +/* aon apb syscon */ +#define IPA_ACCESS_CFG 0xcd8 +#define AON_ACCESS_PCIE_EN BIT(1) + +/* pmu apb syscon */ +#define SNPS_PCIE3_SLP_CTRL 0xac +#define PERST_N_ASSERT BIT(1) +#define PERST_N_AUTO_EN BIT(0) +#define PD_PCIE_CFG_0 0x3e8 +#define PCIE_FORCE_SHUTDOWN BIT(25) + +#define PCIE_SS_REG_BASE 0xE00 +#define APB_CLKFREQ_TIMEOUT 0x4 +#define BUSERR_EN BIT(12) +#define APB_TIMER_DIS BIT(10) +#define APB_TIMER_LIMIT GENMASK(31, 16) + +#define PE0_GEN_CTRL_3 0x58 +#define LTSSM_EN BIT(0) + +struct sprd_pcie_soc_data { +u32 syscon_offset; +}; + +static const struct sprd_pcie_soc_data ums9520_syscon_data = { + .syscon_offset =0x1000 ,/* The offset of set/clear register */ +}; + +struct sprd_pcie { +u32 syscon_offset; +struct device *dev; +struct dw_pcie *pci; +struct regmap *aon_map; +struct regmap *pmu_map; +const struct sprd_pcie_soc_data *socdata; +}; + +enum sprd_pcie_syscon_type { + normal_syscon,/* it's not a set/clear register */
Quoted 8 lines
drivers/pci/controller/dwc/pcie-sprd.c line 66
Quoted 5 lines
drivers/pci/controller/dwc/pcie-sprd.c new file +5
-0+ set_syscon,/* set a set/clear register */ + clr_syscon, /* clear a set/clear register */ +}; + +static void sprd_pcie_buserr_enable (struct dw_pcie *pci)I think you should add a comment to explain what this function enables - it is unclear.
Yes, I agree, maybe also need to change to a more reasonable name :) I will think about it.
Quoted 75 lines
Quoted 75 lines
drivers/pci/controller/dwc/pcie-sprd.c new file +74
-0+{ +u32 val; + + val =dw_pcie_readl_dbi (pci,PCIE_SS_REG_BASE +APB_CLKFREQ_TIMEOUT ); + val &= ~APB_TIMER_DIS ; + val |=BUSERR_EN ; + val |=APB_TIMER_LIMIT & (0x1f4 <<16 ); +dw_pcie_writel_dbi (pci,PCIE_SS_REG_BASE +APB_CLKFREQ_TIMEOUT , val); +} + +static void sprd_pcie_ltssm_enable (struct dw_pcie *pci,bool enable) +{ +u32 val; + + val =dw_pcie_readl_dbi (pci,PCIE_SS_REG_BASE +PE0_GEN_CTRL_3 ); +if (enable) +dw_pcie_writel_dbi (pci,PCIE_SS_REG_BASE +PE0_GEN_CTRL_3 , + val |LTSSM_EN ); +else +dw_pcie_writel_dbi (pci,PCIE_SS_REG_BASE +PE0_GEN_CTRL_3 , + val & ~LTSSM_EN ); +} + +static int sprd_pcie_syscon_set (struct sprd_pcie *ctrl,struct regmap *map, +u32 reg,u32 mask,u32 val, +enum sprd_pcie_syscon_type type) +{ +int ret =0 ; +u32 read_val; +u32 offset = ctrl->syscon_offset; +struct device *dev = ctrl->pci->dev; + +/* +* Each set/clear register has three registers: +* reg: base register +* reg + offset: set register +* reg + offset * 2: clear register +*/ +switch (type) { +case normal_syscon: + ret =regmap_read (map, reg, &read_val); +if (ret) { +dev_err (dev,"failed to read register 0x%x\n" , reg); +return ret; + } + read_val &= ~mask; + read_val |= (val & mask); + ret =regmap_write (map, reg, read_val); +break ; +case set_syscon: + reg = reg + offset; + ret =regmap_write (map, reg, val); +break ; +case clr_syscon: + reg = reg + offset *2 ; + ret =regmap_write (map, reg, val); +break ; +default : +break ; + } + +if (ret) +dev_err (dev,"failed to write register 0x%x\n" , reg); + +return ret; +} + +static int sprd_pcie_perst_assert (struct sprd_pcie *ctrl) +{ +return sprd_pcie_syscon_set (ctrl, ctrl->pmu_map,SNPS_PCIE3_SLP_CTRL , +PERST_N_ASSERT ,PERST_N_ASSERT , set_syscon); +} + +static int sprd_pcie_perst_deassert (struct sprd_pcie *ctrl)
Quoted 8 lines
drivers/pci/controller/dwc/pcie-sprd.c line 146
Quoted 6 lines
drivers/pci/controller/dwc/pcie-sprd.c new file +6
-0+{ +int ret; + + ret =sprd_pcie_syscon_set (ctrl, ctrl->pmu_map,SNPS_PCIE3_SLP_CTRL , +PERST_N_ASSERT ,0 , clr_syscon); +usleep_range (2000 ,3000 );This delay seems arbitrary.
Yes, I guess it came out from some tests result, I need to double check with my colleagues.
Quoted 111 lines
The PCI specifications require a 100ms
after issuing PERST before link training is enabled.Have a look at e.g.
drivers/pci/controller/pci-aardvark.cQuoted 93 lines
drivers/pci/controller/dwc/pcie-sprd.c new file +93
-0+ +return ret; +} + +static int sprd_pcie_power_on (struct platform_device *pdev) +{ +int ret; +struct sprd_pcie *ctrl =platform_get_drvdata (pdev); +struct dw_pcie *pci = ctrl->pci; + + ret =sprd_pcie_syscon_set (ctrl, ctrl->aon_map,PD_PCIE_CFG_0 , +PCIE_FORCE_SHUTDOWN ,0 , clr_syscon); +if (ret) +return ret; + + ret =sprd_pcie_syscon_set (ctrl, ctrl->aon_map,IPA_ACCESS_CFG , +AON_ACCESS_PCIE_EN ,AON_ACCESS_PCIE_EN , + set_syscon); +if (ret) +return ret; + + ret =sprd_pcie_perst_deassert (ctrl); +if (ret) +return ret; + +sprd_pcie_buserr_enable (pci); +sprd_pcie_ltssm_enable (pci, true); + +return ret; +} + +static int sprd_pcie_power_off (struct platform_device *pdev) +{ +struct sprd_pcie *ctrl =platform_get_drvdata (pdev); +struct dw_pcie *pci = ctrl->pci; + +sprd_pcie_ltssm_enable (pci, false); + +sprd_pcie_perst_assert (ctrl); +sprd_pcie_syscon_set (ctrl, ctrl->aon_map,PD_PCIE_CFG_0 , +PCIE_FORCE_SHUTDOWN ,PCIE_FORCE_SHUTDOWN , + set_syscon); +sprd_pcie_syscon_set (ctrl, ctrl->aon_map,IPA_ACCESS_CFG , +AON_ACCESS_PCIE_EN ,0 , clr_syscon); + +return 0 ; +} + +static int sprd_add_pcie_port (struct platform_device *pdev) +{ +struct sprd_pcie *ctrl =platform_get_drvdata (pdev); +struct dw_pcie *pci = ctrl->pci; +struct pcie_port *pp = &pci->pp; + +return dw_pcie_host_init (pp); +} + +static int sprd_pcie_probe (struct platform_device *pdev) +{ +struct device *dev = &pdev->dev; +struct sprd_pcie *ctrl; +struct dw_pcie *pci; +int ret; + + ctrl =devm_kzalloc (dev,sizeof (*ctrl),GFP_KERNEL ); +if (!ctrl) +return -ENOMEM ; + + ctrl->socdata = + (struct sprd_pcie_soc_data *)of_device_get_match_data (dev); +if (!ctrl->socdata) { +dev_warn (dev, +"using the default set/clear register offset address" ); + ctrl->syscon_offset =0x1000 ; + } + ctrl->syscon_offset = ctrl->socdata->syscon_offset; + + ctrl->aon_map =syscon_regmap_lookup_by_phandle (dev->of_node, +"sprd, regmap-aon" ); +if (IS_ERR (ctrl->aon_map)) { +dev_err (dev,"failed to get syscon regmap aon\n" ); + ret =PTR_ERR (ctrl->aon_map); + goto err; + } + + ctrl->pmu_map =syscon_regmap_lookup_by_phandle (dev->of_node, +"sprd, regmap-pmu" ); +if (IS_ERR (ctrl->pmu_map)) { +dev_err (dev,"failed to get syscon regmap pmu\n" ); + ret =PTR_ERR (ctrl->pmu_map); + goto err; + } +drivers/pci/controller/dwc/pcie-sprd.c line 249
Quoted 10 lines
drivers/pci/controller/dwc/pcie-sprd.c new file +10
-0+ pci = ctrl->pci; + pci->dev = dev; + +platform_set_drvdata (pdev, ctrl); + + ret =sprd_pcie_power_on (pdev); +if (ret <0 ) { +dev_err (dev,"failed to power on, return %d\n" , + ret); + goto err_power_off;This looks a bit weird but I believe that's just a naming convention.
Quoted 5 lines
It looks like if we *fail* to power the controller on we power it off, which does not make much sense.
I guess the goal in
err_power_offis to clean-up what was done insprd_pcie_power_on()- more below.
Yes, maybe we need to give it a more suitable name as well.
Quoted 11 lines
drivers/pci/controller/dwc/pcie-sprd.c line 255
Quoted 7 lines
drivers/pci/controller/dwc/pcie-sprd.c new file +6
-0+ } + + ret =sprd_add_pcie_port (pdev); +if (ret) { +dev_warn (dev,"failed to initialize RC controller\n" ); +return ret;If we go by the same logic, we shouldn't return but rather
goto
err_power_off;
Right, I will address this.
Many thanks for your review,
Chunyan
Quoted 46 lines
on error here. Am I missing something ?
Lorenzo
Quoted 41 lines
drivers/pci/controller/dwc/pcie-sprd.c new file +38
-1+ } + +return 0 ; + +err_power_off: +sprd_pcie_power_off (pdev); +err: +return ret; +} + +static int sprd_pcie_remove (struct platform_device *pdev) +{ +sprd_pcie_power_off (pdev); + +return 0 ; +} + +static const struct of_device_id sprd_pcie_of_match[] = { + { + .compatible ="sprd,ums9520-pcie" , + .data = &ums9520_syscon_data, + }, + {}, +}; + +static struct platform_driver sprd_pcie_driver = { + .probe = sprd_pcie_probe, + .remove = sprd_pcie_remove, + .driver = { + .name ="sprd-pcie" , + .of_match_table = sprd_pcie_of_match, + }, +}; + +module_platform_driver (sprd_pcie_driver); + +MODULE_DESCRIPTION ("Unisoc PCIe host controller driver" ); +MODULE_LICENSE ("GPL" ); --2.7.4