On Thu, Apr 29, 2021 at 2:32 AM Shawn Lin <shawn.lin@xxxxxxxxxxxxxx> wrote: > > On 2021/4/29 9:42, Peter Geis wrote: > > On Wed, Apr 28, 2021 at 8:46 PM Shawn Lin <shawn.lin@xxxxxxxxxxxxxx> wrote: > >> > >> On 2021/4/29 0:20, Peter Geis wrote: > >>> On Wed, Apr 14, 2021 at 3:05 AM Simon Xue <xxm@xxxxxxxxxxxxxx> wrote: > >>>> > >>>> Add a driver for the DesignWare-based PCIe controller found on > >>>> RK356X. The existing pcie-rockchip-host driver is only used for > >>>> the Rockchip-designed IP found on RK3399. > >>> > >>> Good Afternoon, > >>> > >>> I've encountered a bit of an issue with this driver. > >> > >> I'm sure your issue is GIC ITS support on RK356x platforms, nothing > >> really related to this driver itself. You can also sort out arm-gic > >> workaround patch from downstream vendor tree, if you want MSI support. > > > > I have functional MSI support, some devices do not support MSIs > > however and need legacy INTx. > > I'd like to point out that the downstream patch does not actually work > > on downstream. > > The GFP_DMA32 flag is discarded by the slab allocator, this breaks MSI > > allocation when the PCIe driver probes. > > I hacked together my own version which works but would definitely not > > be accepted for submission. > > > >> > >>> Unfortunately it does not support legacy interrupts, meaning any PCIe > >>> device that doesn't support MSIs will fail to enumerate: > >>> [ 14.932078] pcieport 0000:00:00.0: of_irq_parse_pci: failed with rc=-22 > >>> [ 14.932708] snd_hda_intel 0000:01:00.1: assign IRQ: got 0 > >>> [ 14.933687] snd_hda_intel 0000:01:00.1: enabling device (0000 -> 0002) > >>> [ 14.934317] snd_hda_intel 0000:01:00.1: Disabling MSI > >>> [ 14.934783] snd_hda_intel 0000:01:00.1: Force to snoop mode by module option > >>> [ 14.935534] snd_hda_intel 0000:01:00.1: enabling bus mastering > >>> [ 14.939764] snd_hda_intel 0000:01:00.1: unable to grab IRQ 0, > >>> disabling device > >>> > >>> Are there plans to support legacy interrupts with the rk3568 controllers? > >> > >> Yes, we did，but not now. > > > > This will significantly limit the devices that can work with the controller. > > Is there any functional reason preventing legacy interrupts from working? > > Just because this $subject patch is still in-flight and we are going to > add it later. Still some features are developed but not ready yet. I now have partial INTx functionality, but it seems there is no way to acknowledge the interrupt only mask and unmask them. Is there a register to acknowledge the legacy interrupts? > > > > I've so far not been successful plugging in support. > > > >> > >> I really don't understand what is blocking this driver from landing > >> mainline? I didn't find any comments from maintainers yet since v1. And > >> now the driver has been iterating for a long time.I believe Simon has > >> addressed all the review feedback during v1 to v7. Can we please land > >> this basic support first. If you can test it with arm-gic workaround, > >> a tested-by tag would be appreciated. > > > > Do you have the DT bindings for this patch available? > > Yes we have, and you can also find it from vendor tree. New drivers should include .yaml documentation files. Downstream does not have one, and one was not included with this patch. The designware-pcie.txt doesn't cover vendor specific implementations. See Documentation/devicetree/bindings/pci/intel-gw-pcie.yaml for example. > > > > > Yes, at this stage what is implemented appears to function correctly, so: > > Tested-by: Peter Geis <pgwipeout@xxxxxxxxx> > > on the rk3566-quartz64 prototype development board. > > Thanks for testing. > > > > > Dependent on the following patches: > > GICv3-ITS official errata for 32bit limitation and shareability limitation. > > Combo-phy support, no patches have been released to mainline yet > > >> > >>> > >>> Thanks, > >>> Peter Geis > >>> > >>>> > >>>> Signed-off-by: Simon Xue <xxm@xxxxxxxxxxxxxx> > >>>> Signed-off-by: Shawn Lin <shawn.lin@xxxxxxxxxxxxxx> > >>>> --- > >>>> drivers/pci/controller/dwc/Kconfig | 10 + > >>>> drivers/pci/controller/dwc/Makefile | 1 + > >>>> drivers/pci/controller/dwc/pcie-dw-rockchip.c | 277 ++++++++++++++++++ > >>>> 3 files changed, 288 insertions(+) > >>>> create mode 100644 drivers/pci/controller/dwc/pcie-dw-rockchip.c > >>>> > >>>> diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig > >>>> index 423d35872ce4..8ab027ba8c04 100644 > >>>> --- a/drivers/pci/controller/dwc/Kconfig > >>>> +++ b/drivers/pci/controller/dwc/Kconfig > >>>> @@ -214,6 +214,16 @@ config PCIE_ARTPEC6_EP > >>>> Enables support for the PCIe controller in the ARTPEC-6 SoC to work in > >>>> endpoint mode. This uses the DesignWare core. > >>>> > >>>> +config PCIE_ROCKCHIP_DW_HOST > >>>> + bool "Rockchip DesignWare PCIe controller" > >>>> + select PCIE_DW > >>>> + select PCIE_DW_HOST > >>>> + depends on ARCH_ROCKCHIP || COMPILE_TEST > >>>> + depends on OF > >>>> + help > >>>> + Enables support for the DesignWare PCIe controller in the > >>>> + Rockchip SoC except RK3399. > >>>> + > >>>> config PCIE_INTEL_GW > >>>> bool "Intel Gateway PCIe host controller support" > >>>> depends on OF && (X86 || COMPILE_TEST) > >>>> diff --git a/drivers/pci/controller/dwc/Makefile b/drivers/pci/controller/dwc/Makefile > >>>> index 952d01941f23..0104659dfe88 100644 > >>>> --- a/drivers/pci/controller/dwc/Makefile > >>>> +++ b/drivers/pci/controller/dwc/Makefile > >>>> @@ -14,6 +14,7 @@ obj-$(CONFIG_PCI_LAYERSCAPE_EP) += pci-layerscape-ep.o > >>>> obj-$(CONFIG_PCIE_QCOM) += pcie-qcom.o > >>>> obj-$(CONFIG_PCIE_ARMADA_8K) += pcie-armada8k.o > >>>> obj-$(CONFIG_PCIE_ARTPEC6) += pcie-artpec6.o > >>>> +obj-$(CONFIG_PCIE_ROCKCHIP_DW_HOST) += pcie-dw-rockchip.o > >>>> obj-$(CONFIG_PCIE_INTEL_GW) += pcie-intel-gw.o > >>>> obj-$(CONFIG_PCIE_KIRIN) += pcie-kirin.o > >>>> obj-$(CONFIG_PCIE_HISI_STB) += pcie-histb.o > >>>> diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c > >>>> new file mode 100644 > >>>> index 000000000000..3f060144eeab > >>>> --- /dev/null > >>>> +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c > >>>> @@ -0,0 +1,277 @@ > >>>> +// SPDX-License-Identifier: GPL-2.0 > >>>> +/* > >>>> + * PCIe host controller driver for Rockchip SoCs. > >>>> + * > >>>> + * Copyright (C) 2021 Rockchip Electronics Co., Ltd. > >>>> + * http://www.rock-chips.com > >>>> + * > >>>> + * Author: Simon Xue <xxm@xxxxxxxxxxxxxx> > >>>> + */ > >>>> + > >>>> +#include <linux/clk.h> > >>>> +#include <linux/gpio/consumer.h> > >>>> +#include <linux/mfd/syscon.h> > >>>> +#include <linux/module.h> > >>>> +#include <linux/of_device.h> > >>>> +#include <linux/phy/phy.h> > >>>> +#include <linux/platform_device.h> > >>>> +#include <linux/regmap.h> > >>>> +#include <linux/reset.h> > >>>> + > >>>> +#include "pcie-designware.h" > >>>> + > >>>> +/* > >>>> + * The upper 16 bits of PCIE_CLIENT_CONFIG are a write > >>>> + * mask for the lower 16 bits. > >>>> + */ > >>>> +#define HIWORD_UPDATE(mask, val) (((mask) << 16) | (val)) > >>>> +#define HIWORD_UPDATE_BIT(val) HIWORD_UPDATE(val, val) > >>>> + > >>>> +#define to_rockchip_pcie(x) dev_get_drvdata((x)->dev) > >>>> + > >>>> +#define PCIE_CLIENT_RC_MODE HIWORD_UPDATE_BIT(0x40) > >>>> +#define PCIE_CLIENT_ENABLE_LTSSM HIWORD_UPDATE_BIT(0xc) > >>>> +#define PCIE_SMLH_LINKUP BIT(16) > >>>> +#define PCIE_RDLH_LINKUP BIT(17) > >>>> +#define PCIE_LINKUP (PCIE_SMLH_LINKUP | PCIE_RDLH_LINKUP) > >>>> +#define PCIE_L0S_ENTRY 0x11 > >>>> +#define PCIE_CLIENT_GENERAL_CONTROL 0x0 > >>>> +#define PCIE_CLIENT_GENERAL_DEBUG 0x104 > >>>> +#define PCIE_CLIENT_HOT_RESET_CTRL 0x180 > >>>> +#define PCIE_CLIENT_LTSSM_STATUS 0x300 > >>>> +#define PCIE_LTSSM_ENABLE_ENHANCE BIT(4) > >>>> + > >>>> +struct rockchip_pcie { > >>>> + struct dw_pcie pci; > >>>> + void __iomem *apb_base; > >>>> + struct phy *phy; > >>>> + struct clk_bulk_data *clks; > >>>> + unsigned int clk_cnt; > >>>> + struct reset_control *rst; > >>>> + struct gpio_desc *rst_gpio; > >>>> + struct regulator *vpcie3v3; > >>>> +}; > >>>> + > >>>> +static int rockchip_pcie_readl_apb(struct rockchip_pcie *rockchip, > >>>> + u32 reg) > >>>> +{ > >>>> + return readl(rockchip->apb_base + reg); > >>>> +} > >>>> + > >>>> +static void rockchip_pcie_writel_apb(struct rockchip_pcie *rockchip, > >>>> + u32 val, u32 reg) > >>>> +{ > >>>> + writel(val, rockchip->apb_base + reg); > >>>> +} > >>>> + > >>>> +static void rockchip_pcie_enable_ltssm(struct rockchip_pcie *rockchip) > >>>> +{ > >>>> + rockchip_pcie_writel_apb(rockchip, PCIE_CLIENT_ENABLE_LTSSM, > >>>> + PCIE_CLIENT_GENERAL_CONTROL); > >>>> +} > >>>> + > >>>> +static int rockchip_pcie_link_up(struct dw_pcie *pci) > >>>> +{ > >>>> + struct rockchip_pcie *rockchip = to_rockchip_pcie(pci); > >>>> + u32 val = rockchip_pcie_readl_apb(rockchip, PCIE_CLIENT_LTSSM_STATUS); > >>>> + > >>>> + if ((val & (PCIE_RDLH_LINKUP | PCIE_SMLH_LINKUP)) == PCIE_LINKUP && > >>>> + (val & GENMASK(5, 0)) == PCIE_L0S_ENTRY) > >>>> + return 1; > >>>> + > >>>> + return 0; > >>>> +} > >>>> + > >>>> +static int rockchip_pcie_start_link(struct dw_pcie *pci) > >>>> +{ > >>>> + struct rockchip_pcie *rockchip = to_rockchip_pcie(pci); > >>>> + > >>>> + /* Reset device */ > >>>> + gpiod_set_value_cansleep(rockchip->rst_gpio, 0); > >>>> + > >>>> + rockchip_pcie_enable_ltssm(rockchip); > >>>> + > >>>> + /* > >>>> + * PCIe requires the refclk to be stable for 100µs prior to releasing > >>>> + * PERST. See table 2-4 in section 2.6.2 AC Specifications of the PCI > >>>> + * Express Card Electromechanical Specification, 1.1. However, we don't > >>>> + * know if the refclk is coming from RC's PHY or external OSC. If it's > >>>> + * from RC, so enabling LTSSM is the just right place to release #PERST. > >>>> + * We need more extra time as before, rather than setting just > >>>> + * 100us as we don't know how long should the device need to reset. > >>>> + */ > >>>> + msleep(100); > >>>> + gpiod_set_value_cansleep(rockchip->rst_gpio, 1); > >>>> + > >>>> + return 0; > >>>> +} > >>>> + > >>>> +static int rockchip_pcie_host_init(struct pcie_port *pp) > >>>> +{ > >>>> + struct dw_pcie *pci = to_dw_pcie_from_pp(pp); > >>>> + struct rockchip_pcie *rockchip = to_rockchip_pcie(pci); > >>>> + u32 val; > >>>> + > >>>> + /* LTSSM enable control mode */ > >>>> + val = rockchip_pcie_readl_apb(rockchip, PCIE_CLIENT_HOT_RESET_CTRL); > >>>> + val |= PCIE_LTSSM_ENABLE_ENHANCE | (PCIE_LTSSM_ENABLE_ENHANCE << 16); > >>>> + rockchip_pcie_writel_apb(rockchip, val, PCIE_CLIENT_HOT_RESET_CTRL); > >>>> + > >>>> + rockchip_pcie_writel_apb(rockchip, PCIE_CLIENT_RC_MODE, > >>>> + PCIE_CLIENT_GENERAL_CONTROL); > >>>> + > >>>> + return 0; > >>>> +} > >>>> + > >>>> +static const struct dw_pcie_host_ops rockchip_pcie_host_ops = { > >>>> + .host_init = rockchip_pcie_host_init, > >>>> +}; > >>>> + > >>>> +static int rockchip_pcie_clk_init(struct rockchip_pcie *rockchip) > >>>> +{ > >>>> + struct device *dev = rockchip->pci.dev; > >>>> + int ret; > >>>> + > >>>> + ret = devm_clk_bulk_get_all(dev, &rockchip->clks); > >>>> + if (ret < 0) > >>>> + return ret; > >>>> + > >>>> + rockchip->clk_cnt = ret; > >>>> + > >>>> + return clk_bulk_prepare_enable(rockchip->clk_cnt, rockchip->clks); > >>>> +} > >>>> + > >>>> +static int rockchip_pcie_resource_get(struct platform_device *pdev, > >>>> + struct rockchip_pcie *rockchip) > >>>> +{ > >>>> + rockchip->apb_base = devm_platform_ioremap_resource_byname(pdev, "apb"); > >>>> + if (IS_ERR(rockchip->apb_base)) > >>>> + return PTR_ERR(rockchip->apb_base); > >>>> + > >>>> + rockchip->rst_gpio = devm_gpiod_get_optional(&pdev->dev, "reset", > >>>> + GPIOD_OUT_HIGH); > >>>> + if (IS_ERR(rockchip->rst_gpio)) > >>>> + return PTR_ERR(rockchip->rst_gpio); > >>>> + > >>>> + return 0; > >>>> +} > >>>> + > >>>> +static int rockchip_pcie_phy_init(struct rockchip_pcie *rockchip) > >>>> +{ > >>>> + struct device *dev = rockchip->pci.dev; > >>>> + int ret; > >>>> + > >>>> + rockchip->phy = devm_phy_get(dev, "pcie-phy"); > >>>> + if (IS_ERR(rockchip->phy)) > >>>> + return dev_err_probe(dev, PTR_ERR(rockchip->phy), > >>>> + "missing PHY\n"); > >>>> + > >>>> + ret = phy_init(rockchip->phy); > >>>> + if (ret < 0) > >>>> + return ret; > >>>> + > >>>> + ret = phy_power_on(rockchip->phy); > >>>> + if (ret) > >>>> + phy_exit(rockchip->phy); > >>>> + > >>>> + return ret; > >>>> +} > >>>> + > >>>> +static void rockchip_pcie_phy_deinit(struct rockchip_pcie *rockchip) > >>>> +{ > >>>> + phy_exit(rockchip->phy); > >>>> + phy_power_off(rockchip->phy); > >>>> +} > >>>> + > >>>> +static int rockchip_pcie_reset_control_release(struct rockchip_pcie *rockchip) > >>>> +{ > >>>> + struct device *dev = rockchip->pci.dev; > >>>> + > >>>> + rockchip->rst = devm_reset_control_array_get_exclusive(dev); > >>>> + if (IS_ERR(rockchip->rst)) > >>>> + return dev_err_probe(dev, PTR_ERR(rockchip->rst), > >>>> + "failed to get reset lines\n"); > >>>> + > >>>> + return reset_control_deassert(rockchip->rst); > >>>> +} > >>>> + > >>>> +static const struct dw_pcie_ops dw_pcie_ops = { > >>>> + .link_up = rockchip_pcie_link_up, > >>>> + .start_link = rockchip_pcie_start_link, > >>>> +}; > >>>> + > >>>> +static int rockchip_pcie_probe(struct platform_device *pdev) > >>>> +{ > >>>> + struct device *dev = &pdev->dev; > >>>> + struct rockchip_pcie *rockchip; > >>>> + struct pcie_port *pp; > >>>> + int ret; > >>>> + > >>>> + rockchip = devm_kzalloc(dev, sizeof(*rockchip), GFP_KERNEL); > >>>> + if (!rockchip) > >>>> + return -ENOMEM; > >>>> + > >>>> + platform_set_drvdata(pdev, rockchip); > >>>> + > >>>> + rockchip->pci.dev = dev; > >>>> + rockchip->pci.ops = &dw_pcie_ops; > >>>> + > >>>> + pp = &rockchip->pci.pp; > >>>> + pp->ops = &rockchip_pcie_host_ops; > >>>> + > >>>> + ret = rockchip_pcie_resource_get(pdev, rockchip); > >>>> + if (ret) > >>>> + return ret; > >>>> + > >>>> + /* DON'T MOVE ME: must be enable before PHY init */ > >>>> + rockchip->vpcie3v3 = devm_regulator_get_optional(dev, "vpcie3v3"); > >>>> + if (IS_ERR(rockchip->vpcie3v3)) > >>>> + if (PTR_ERR(rockchip->vpcie3v3) != -ENODEV) > >>>> + return dev_err_probe(dev, PTR_ERR(rockchip->vpcie3v3), > >>>> + "failed to get vpcie3v3 regulator\n"); > >>>> + > >>>> + ret = regulator_enable(rockchip->vpcie3v3); > >>>> + if (ret) { > >>>> + dev_err(dev, "failed to enable vpcie3v3 regulator\n"); > >>>> + return ret; > >>>> + } > >>>> + > >>>> + ret = rockchip_pcie_phy_init(rockchip); > >>>> + if (ret) > >>>> + goto disable_regulator; > >>>> + > >>>> + ret = rockchip_pcie_reset_control_release(rockchip); > >>>> + if (ret) > >>>> + goto deinit_phy; > >>>> + > >>>> + ret = rockchip_pcie_clk_init(rockchip); > >>>> + if (ret) > >>>> + goto deinit_phy; > >>>> + > >>>> + ret = dw_pcie_host_init(pp); > >>>> + if (!ret) > >>>> + return 0; > >>>> + > >>>> + clk_bulk_disable_unprepare(rockchip->clk_cnt, rockchip->clks); > >>>> +deinit_phy: > >>>> + rockchip_pcie_phy_deinit(rockchip); > >>>> +disable_regulator: > >>>> + regulator_disable(rockchip->vpcie3v3); > >>>> + > >>>> + return ret; > >>>> +} > >>>> + > >>>> +static const struct of_device_id rockchip_pcie_of_match[] = { > >>>> + { .compatible = "rockchip,rk3568-pcie", }, > >>>> + {}, > >>>> +}; > >>>> + > >>>> +static struct platform_driver rockchip_pcie_driver = { > >>>> + .driver = { > >>>> + .name = "rockchip-dw-pcie", > >>>> + .of_match_table = rockchip_pcie_of_match, > >>>> + .suppress_bind_attrs = true, > >>>> + }, > >>>> + .probe = rockchip_pcie_probe, > >>>> +}; > >>>> +builtin_platform_driver(rockchip_pcie_driver); > >>>> -- > >>>> 2.25.1 > >>>> > >>>> > >>>> > >>>> > >>>> _______________________________________________ > >>>> Linux-rockchip mailing list > >>>> Linux-rockchip@xxxxxxxxxxxxxxxxxxx > >>>> http://lists.infradead.org/mailman/listinfo/linux-rockchip > >>> > >>> > >>> > >> > >> > > > > > > > >
