| Message ID | 1463740156-7148-1-git-send-email-shawn.lin@rock-chips.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
Hi Shawn, I haven't had any contact with PCI yet, so my comments below will likely address more generic findings only. As you might've guessed from the binding comments, to me it looks like the phy handling should be in a separate phy driver and looking below all phy accesses seem very separate from the actual pci controller interactions - they are even in port_init only as well. While I already added some comments about that below, the driver seems full of raw register bit handling (including wild shifts). Please abstract that using contants, so that stuff stays readable for the future. Am Freitag, 20. Mai 2016, 18:29:16 schrieb Shawn Lin: > RK3399 has a PCIe controller which can be used as Root Complex. > This driver supports a PCIe controller as Root Complex mode. > > Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com> > --- [...] > diff --git a/drivers/pci/host/pcie-rockchip.c > b/drivers/pci/host/pcie-rockchip.c new file mode 100644 > index 0000000..4063fd3 > --- /dev/null > +++ b/drivers/pci/host/pcie-rockchip.c > @@ -0,0 +1,1181 @@ > +/* > + * Rockchip AXI PCIe host controller driver > + * > + * Copyright (c) 2016 Rockchip, Inc. > + * > + * Author: Shawn Lin <shawn.lin@rock-chips.com> > + * Wenrui Li <wenrui.li@rock-chips.com> > + * Bits taken from Synopsys Designware Host controller driver and > + * ARM PCI Host generic driver. > + * > + * This program is free software: you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation, either version 2 of the License, or > + * (at your option) any later version. > + */ > +#include <linux/clk.h> > +#include <linux/delay.h> > +#include <linux/gpio/consumer.h> > +#include <linux/interrupt.h> > +#include <linux/irq.h> > +#include <linux/irqdomain.h> > +#include <linux/kernel.h> > +#include <linux/mfd/syscon.h> > +#include <linux/module.h> > +#include <linux/msi.h> > +#include <linux/of_address.h> > +#include <linux/of_device.h> > +#include <linux/of_pci.h> > +#include <linux/of_platform.h> > +#include <linux/of_irq.h> > +#include <linux/pci.h> > +#include <linux/platform_device.h> > +#include <linux/reset.h> > +#include <linux/regmap.h> > + > +#define REF_CLK_100MHZ (100 * 1000 * 1000) seems unused > +#define PCIE_CLIENT_BASE 0x0 > +#define PCIE_RC_CONFIG_BASE 0xa00000 > +#define PCIE_CORE_CTRL_MGMT_BASE 0x900000 > +#define PCIE_CORE_AXI_CONF_BASE 0xc00000 > +#define PCIE_CORE_AXI_INBOUND_BASE 0xc00800 > + > +#define PCIE_CLIENT_BASIC_STATUS0 0x44 > +#define PCIE_CLIENT_BASIC_STATUS1 0x48 > +#define PCIE_CLIENT_INT_MASK 0x4c > +#define PCIE_CLIENT_INT_STATUS 0x50 > +#define PCIE_CORE_INT_MASK 0x900210 > +#define PCIE_CORE_INT_STATUS 0x90020c > + > +/** Size of one AXI Region (not Region 0) */ > +#define AXI_REGION_SIZE (0x1 << 20) for those generic (1 << x) things please use BIT(x) instead Also constants intertwined with constants is hard to read, so ideally add a blank line above each comment and comment style for single line comments only has one * in the beginning /* foo */ > +/** Overall size of AXI area */ > +#define AXI_OVERALL_SIZE (64 * (0x1 << 20)) > +/** Size of Region 0, equal to sum of sizes of other regions */ > +#define AXI_REGION_0_SIZE (32 * (0x1 << 20)) > +#define OB_REG_SIZE_SHIFT 5 > +#define IB_ROOT_PORT_REG_SIZE_SHIFT 3 > + > +#define AXI_WRAPPER_IO_WRITE 0x6 > +#define AXI_WRAPPER_MEM_WRITE 0x2 > +#define MAX_AXI_IB_ROOTPORT_REGION_NUM 3 > +#define MIN_AXI_ADDR_BITS_PASSED 8 strange spacing after #define [...] > +/** > + * rockchip_pcie_init_port - Initialize hardware > + * @port: PCIe port information > + */ > +static int rockchip_pcie_init_port(struct rockchip_pcie_port *port) > +{ > + int err; > + u32 status; > + unsigned long timeout = jiffies + msecs_to_jiffies(1000); this timeout only seems to be initialized once here but used for multiple loops below resulting in all waitloops combined together being allowed to take 1 second ... is this intended? > + gpiod_set_value(port->ep_gpio, 0); > + > + /* Make sure PCIe relate block is in reset state */ > + err = reset_control_assert(port->phy_rst); > + if (err) { > + dev_err(port->dev, "assert phy_rst err %d\n", err); > + return err; > + } should move to phy driver probe or so > + err = reset_control_assert(port->core_rst); > + if (err) { > + dev_err(port->dev, "assert core_rst err %d\n", err); > + return err; > + } blank line > + err = reset_control_assert(port->mgmt_rst); > + if (err) { > + dev_err(port->dev, "assert mgmt_rst err %d\n", err); > + return err; > + } blank line > + err = reset_control_assert(port->mgmt_sticky_rst); > + if (err) { > + dev_err(port->dev, "assert mgmt_sticky_rst err %d\n", err); > + return err; > + } blank line > + err = reset_control_assert(port->pipe_rst); > + if (err) { > + dev_err(port->dev, "assert pipe_rst err %d\n", err); > + return err; > + } > + > + pcie_write(port, (0xf << 20) | (0x1 << 16) | PCIE_CLIENT_GEN_SEL_2 | > + (0x1 << 19) | (0x1 << 3) | > + PCIE_CLIENT_MODE_RC | > + PCIE_CLIENT_CONF_LANE_NUM(port->lanes) | > + PCIE_CLIENT_CONF_ENABLE, PCIE_CLIENT_BASE); > + ------ phy_enable begin ------ As mentioned above, the whole phy handling seems pretty separate, so should be easily being able to live in a real phy driver? This of course also needs to handle the phy clock in it. > + err = reset_control_deassert(port->phy_rst); > + if (err) { > + dev_err(port->dev, "deassert phy_rst err %d\n", err); > + return err; > + } > + regmap_write(port->grf, port->pcie_conf, > + (0x3f << 17) | (0x10 << 1)); the bits above should use some sort of constant / description. Also see HIWORD_UPDATE in other drivers for the write-enable mask also add blank line here > + err = -EINVAL; > + while (time_before(jiffies, timeout)) { > + regmap_read(port->grf, port->pcie_status, &status); > + if ((status & (1 << 9))) { use a constant for the (1 << 9) [aka BIT(9)] please > + dev_info(port->dev, "pll locked!\n"); dev_dbg instead? > + err = 0; > + break; > + } > + } > + if (err) { > + dev_err(port->dev, "pll lock timeout!\n"); > + return err; > + } > + pcie_pb_wr_cfg(port, 0x10, 0x8); > + pcie_pb_wr_cfg(port, 0x12, 0x8); > + > + err = -ETIMEDOUT; > + while (time_before(jiffies, timeout)) { > + regmap_read(port->grf, port->pcie_status, &status); > + if (!(status & (1 << 10))) { constant again > + dev_info(port->dev, "pll output enable done!\n"); dev_dbg or leave it out > + err = 0; > + break; > + } > + } > + > + if (err) { > + dev_err(port->dev, "pll output enable timeout!\n"); > + return err; > + } > + > + regmap_write(port->grf, port->pcie_conf, > + (0x3f << 17) | (0x10 << 1)); > + err = -EINVAL; > + while (time_before(jiffies, timeout)) { > + regmap_read(port->grf, port->pcie_status, &status); > + if ((status & (1 << 9))) { > + dev_info(port->dev, "pll relocked!\n"); > + err = 0; > + break; > + } > + } > + if (err) { > + dev_err(port->dev, "pll relock timeout!\n"); > + return err; > + } ------ phy_enable end ------ > + err = reset_control_deassert(port->core_rst); > + if (err) { > + dev_err(port->dev, "deassert core_rst err %d\n", err); > + return err; > + } > + err = reset_control_deassert(port->mgmt_rst); > + if (err) { > + dev_err(port->dev, "deassert mgmt_rst err %d\n", err); > + return err; > + } > + err = reset_control_deassert(port->mgmt_sticky_rst); > + if (err) { > + dev_err(port->dev, "deassert mgmt_sticky_rst err %d\n", err); > + return err; > + } > + err = reset_control_deassert(port->pipe_rst); > + if (err) { > + dev_err(port->dev, "deassert pipe_rst err %d\n", err); > + return err; > + } > + > + pcie_write(port, 1 << 17 | 1 << 1, PCIE_CLIENT_BASE); > + > + gpiod_set_value(port->ep_gpio, 1); > + err = -ETIMEDOUT; > + while (time_before(jiffies, timeout)) { > + status = pcie_read(port, PCIE_CLIENT_BASIC_STATUS1); > + if (((status >> 20) & 0x3) == 0x3) { > + dev_info(port->dev, "pcie link training gen1 pass!\n"); > + err = 0; > + break; > + } > + } > + if (err) { > + dev_err(port->dev, "pcie link training gen1 timeout!\n"); > + return err; > + } > + > + status = pcie_read(port, 0x9000d0); > + status |= 0x20; > + pcie_write(port, status, 0x9000d0); just to mention it again, bit handling as descriptive constant please and also please give that register a name :-) [...] > +static int rockchip_pcie_parse_dt(struct rockchip_pcie_port *port) > +{ [...] > + port->lanes = 1; > + err = of_property_read_u32(node, "num-lanes", &port->lanes); > + if (!err && ((port->lanes == 0) || > + (port->lanes == 3) || > + (port->lanes > 4))) { > + dev_info(dev, "invalid num-lanes, default use one lane\n"); dev_warn might be more appropriate > + port->lanes = 1; > + } [...] > +static void rockchip_pcie_msi_enable(struct rockchip_pcie_port *pp) > +{ > + struct device_node *msi_node; > + > + msi_node = of_parse_phandle(pp->dev->of_node, > + "msi-parent", 0); I would assume this should live in the general parse_dt function? Also is that MSI enablement really allow to fail silently without any affect on the PCI functionality? > + if (!msi_node) > + return; > + > + pp->msi = of_pci_find_msi_chip_by_node(msi_node); > + of_node_put(msi_node); > + > + if (pp->msi) > + pp->msi->dev = pp->dev; > +} [...] > +static int rockchip_pcie_probe(struct platform_device *pdev) > +{ > + struct rockchip_pcie_port *port; > + struct device *dev = &pdev->dev; > + struct pci_bus *bus, *child; > + struct resource_entry *win; > + int reg_no; > + int err = 0; > + int irq; > + LIST_HEAD(res); > + > + if (!dev->of_node) > + return -ENODEV; > + > + port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL); > + if (!port) > + return -ENOMEM; > + > + irq = platform_get_irq_byname(pdev, "pcie-sys"); > + if (irq < 0) { > + dev_err(dev, "missing pcie_sys IRQ resource\n"); > + return -EINVAL; > + } blank line > + err = devm_request_irq(dev, irq, rockchip_pcie_subsys_irq_handler, > + IRQF_SHARED, "pcie-sys", port); > + if (err) { > + dev_err(dev, "failed to request pcie subsystem irq\n"); > + return err; > + } > + > + port->irq = platform_get_irq_byname(pdev, "pcie-legacy"); > + if (port->irq < 0) { > + dev_err(dev, "missing pcie_legacy IRQ resource\n"); > + return -EINVAL; > + } blank line > + err = devm_request_irq(dev, port->irq, > + rockchip_pcie_legacy_int_handler, > + IRQF_SHARED, > + "pcie-legacy", > + port); > + if (err) { > + dev_err(&pdev->dev, "failed to request pcie-legacy irq\n"); > + return err; > + } > + > + irq = platform_get_irq_byname(pdev, "pcie-client"); > + if (irq < 0) { > + dev_err(dev, "missing pcie-client IRQ resource\n"); > + return -EINVAL; > + } blank line > + err = devm_request_irq(dev, irq, rockchip_pcie_client_irq_handler, > + IRQF_SHARED, "pcie-client", port); > + if (err) { > + dev_err(dev, "failed to request pcie client irq\n"); > + return err; > + } > + > + port->dev = dev; > + err = rockchip_pcie_parse_dt(port); > + if (err) { > + dev_err(dev, "Parsing DT failed\n"); > + return err; > + } rockchip_pcie_parse_dt already emits error messages that are a lot more specific to the actual problem, so you don't need another error message here and can just return the error code. > + err = rockchip_pcie_init_port(port); > + if (err) > + return err; > + > + platform_set_drvdata(pdev, port); > + > + rockchip_pcie_enable_interrupts(port); > + if (!IS_ENABLED(CONFIG_PCI_MSI)) { > + err = rockchip_pcie_init_irq_domain(port); > + if (err < 0) > + return err; > + } > + > + err = of_pci_get_host_bridge_resources(dev->of_node, 0, 0xff, > + &res, &port->io_base); > + if (err) > + return err; add blank line Heiko -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2016/5/21 5:13, Heiko Stuebner wrote: > Hi Shawn, > > I haven't had any contact with PCI yet, so my comments below will likely > address more generic findings only. > > As you might've guessed from the binding comments, to me it looks like the > phy handling should be in a separate phy driver and looking below all phy > accesses seem very separate from the actual pci controller interactions - > they are even in port_init only as well. > yes, the main reason for not to seperate a new pcie-phy driver for this prototype design is that I just wonder whether it's worth to create a new driver for just a small piece of code. And a bit more forward is that I think phy api is no so scalable for just init/power_on in case of too much interactions between controller and phy from which I suffer a bit for emmc. Should I really need to seperate the phy part into pcie-phy? ;) > While I already added some comments about that below, the driver seems full > of raw register bit handling (including wild shifts). Please abstract that > using contants, so that stuff stays readable for the future. okay, let me migrate these magic number and raw reg bit handling into a new header file. > > > Am Freitag, 20. Mai 2016, 18:29:16 schrieb Shawn Lin: >> RK3399 has a PCIe controller which can be used as Root Complex. >> This driver supports a PCIe controller as Root Complex mode. >> >> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com> >> --- > > [...] > >> diff --git a/drivers/pci/host/pcie-rockchip.c >> b/drivers/pci/host/pcie-rockchip.c new file mode 100644 >> index 0000000..4063fd3 >> --- /dev/null >> +++ b/drivers/pci/host/pcie-rockchip.c >> @@ -0,0 +1,1181 @@ >> +/* >> + * Rockchip AXI PCIe host controller driver >> + * >> + * Copyright (c) 2016 Rockchip, Inc. >> + * >> + * Author: Shawn Lin <shawn.lin@rock-chips.com> >> + * Wenrui Li <wenrui.li@rock-chips.com> >> + * Bits taken from Synopsys Designware Host controller driver and >> + * ARM PCI Host generic driver. >> + * >> + * This program is free software: you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License as published by >> + * the Free Software Foundation, either version 2 of the License, or >> + * (at your option) any later version. >> + */ >> +#include <linux/clk.h> >> +#include <linux/delay.h> >> +#include <linux/gpio/consumer.h> >> +#include <linux/interrupt.h> >> +#include <linux/irq.h> >> +#include <linux/irqdomain.h> >> +#include <linux/kernel.h> >> +#include <linux/mfd/syscon.h> >> +#include <linux/module.h> >> +#include <linux/msi.h> >> +#include <linux/of_address.h> >> +#include <linux/of_device.h> >> +#include <linux/of_pci.h> >> +#include <linux/of_platform.h> >> +#include <linux/of_irq.h> >> +#include <linux/pci.h> >> +#include <linux/platform_device.h> >> +#include <linux/reset.h> >> +#include <linux/regmap.h> >> + >> +#define REF_CLK_100MHZ (100 * 1000 * 1000) > > seems unused will be removed. > >> +#define PCIE_CLIENT_BASE 0x0 >> +#define PCIE_RC_CONFIG_BASE 0xa00000 >> +#define PCIE_CORE_CTRL_MGMT_BASE 0x900000 >> +#define PCIE_CORE_AXI_CONF_BASE 0xc00000 >> +#define PCIE_CORE_AXI_INBOUND_BASE 0xc00800 >> + >> +#define PCIE_CLIENT_BASIC_STATUS0 0x44 >> +#define PCIE_CLIENT_BASIC_STATUS1 0x48 >> +#define PCIE_CLIENT_INT_MASK 0x4c >> +#define PCIE_CLIENT_INT_STATUS 0x50 >> +#define PCIE_CORE_INT_MASK 0x900210 >> +#define PCIE_CORE_INT_STATUS 0x90020c >> + >> +/** Size of one AXI Region (not Region 0) */ >> +#define AXI_REGION_SIZE (0x1 << 20) > > for those generic (1 << x) things please use BIT(x) instead > Also constants intertwined with constants is hard to read, > so ideally add a blank line above each comment and comment style for single > line comments only has one * in the beginning Ahh yep. > /* foo */ > >> +/** Overall size of AXI area */ >> +#define AXI_OVERALL_SIZE (64 * (0x1 << 20)) >> +/** Size of Region 0, equal to sum of sizes of other regions */ >> +#define AXI_REGION_0_SIZE (32 * (0x1 << 20)) >> +#define OB_REG_SIZE_SHIFT 5 >> +#define IB_ROOT_PORT_REG_SIZE_SHIFT 3 >> + >> +#define AXI_WRAPPER_IO_WRITE 0x6 >> +#define AXI_WRAPPER_MEM_WRITE 0x2 >> +#define MAX_AXI_IB_ROOTPORT_REGION_NUM 3 >> +#define MIN_AXI_ADDR_BITS_PASSED 8 > > strange spacing after #define Generally I use one space after #define, but I donnot somehow... I will check it twice. > > [...] > >> +/** >> + * rockchip_pcie_init_port - Initialize hardware >> + * @port: PCIe port information >> + */ >> +static int rockchip_pcie_init_port(struct rockchip_pcie_port *port) >> +{ >> + int err; >> + u32 status; >> + unsigned long timeout = jiffies + msecs_to_jiffies(1000); > > this timeout only seems to be initialized once here but used for multiple > loops below resulting in all waitloops combined together being allowed to > take 1 second ... is this intended? a big timeout value to make sure we leave enough margin. No any data is provided about how long should we wait for pll lock/re-lock/tainning finish stuff. As it also related to the Socs/devices. Currently I test pci-ethernet/wifi/SATA-bridge/USB-bridge, and it seems can be reduced. But as you know, I cannot guarantee not to augment it once we find some SSD/GPU in the failure state of trainning in the future. Anyway I think here we use loop-break method, so it should be not harmful? > >> + gpiod_set_value(port->ep_gpio, 0); >> + >> + /* Make sure PCIe relate block is in reset state */ >> + err = reset_control_assert(port->phy_rst); >> + if (err) { >> + dev_err(port->dev, "assert phy_rst err %d\n", err); >> + return err; >> + } > > should move to phy driver probe or so > >> + err = reset_control_assert(port->core_rst); >> + if (err) { >> + dev_err(port->dev, "assert core_rst err %d\n", err); >> + return err; >> + } > > blank line > >> + err = reset_control_assert(port->mgmt_rst); >> + if (err) { >> + dev_err(port->dev, "assert mgmt_rst err %d\n", err); >> + return err; >> + } > > blank line > >> + err = reset_control_assert(port->mgmt_sticky_rst); >> + if (err) { >> + dev_err(port->dev, "assert mgmt_sticky_rst err %d\n", err); >> + return err; >> + } > > blank line > >> + err = reset_control_assert(port->pipe_rst); >> + if (err) { >> + dev_err(port->dev, "assert pipe_rst err %d\n", err); >> + return err; >> + } >> + >> + pcie_write(port, (0xf << 20) | (0x1 << 16) | PCIE_CLIENT_GEN_SEL_2 | >> + (0x1 << 19) | (0x1 << 3) | >> + PCIE_CLIENT_MODE_RC | >> + PCIE_CLIENT_CONF_LANE_NUM(port->lanes) | >> + PCIE_CLIENT_CONF_ENABLE, PCIE_CLIENT_BASE); >> + > > ------ phy_enable begin ------ > > As mentioned above, the whole phy handling seems pretty separate, so should > be easily being able to live in a real phy driver? > > This of course also needs to handle the phy clock in it. > >> + err = reset_control_deassert(port->phy_rst); >> + if (err) { >> + dev_err(port->dev, "deassert phy_rst err %d\n", err); >> + return err; >> + } >> + regmap_write(port->grf, port->pcie_conf, >> + (0x3f << 17) | (0x10 << 1)); > > the bits above should use some sort of constant / description. Also see > HIWORD_UPDATE in other drivers for the write-enable mask > > also add blank line here > >> + err = -EINVAL; >> + while (time_before(jiffies, timeout)) { >> + regmap_read(port->grf, port->pcie_status, &status); >> + if ((status & (1 << 9))) { > > use a constant for the (1 << 9) [aka BIT(9)] please > >> + dev_info(port->dev, "pll locked!\n"); > > dev_dbg instead? yep > >> + err = 0; >> + break; >> + } >> + } >> + if (err) { >> + dev_err(port->dev, "pll lock timeout!\n"); >> + return err; >> + } >> + pcie_pb_wr_cfg(port, 0x10, 0x8); >> + pcie_pb_wr_cfg(port, 0x12, 0x8); >> + >> + err = -ETIMEDOUT; >> + while (time_before(jiffies, timeout)) { >> + regmap_read(port->grf, port->pcie_status, &status); >> + if (!(status & (1 << 10))) { > > constant again > >> + dev_info(port->dev, "pll output enable done!\n"); > > dev_dbg or leave it out dev_dbg should be fine. > >> + err = 0; >> + break; >> + } >> + } >> + >> + if (err) { >> + dev_err(port->dev, "pll output enable timeout!\n"); >> + return err; >> + } >> + >> + regmap_write(port->grf, port->pcie_conf, >> + (0x3f << 17) | (0x10 << 1)); >> + err = -EINVAL; >> + while (time_before(jiffies, timeout)) { >> + regmap_read(port->grf, port->pcie_status, &status); >> + if ((status & (1 << 9))) { >> + dev_info(port->dev, "pll relocked!\n"); >> + err = 0; >> + break; >> + } >> + } >> + if (err) { >> + dev_err(port->dev, "pll relock timeout!\n"); >> + return err; >> + } > > ------ phy_enable end ------ > > >> + err = reset_control_deassert(port->core_rst); >> + if (err) { >> + dev_err(port->dev, "deassert core_rst err %d\n", err); >> + return err; >> + } >> + err = reset_control_deassert(port->mgmt_rst); >> + if (err) { >> + dev_err(port->dev, "deassert mgmt_rst err %d\n", err); >> + return err; >> + } >> + err = reset_control_deassert(port->mgmt_sticky_rst); >> + if (err) { >> + dev_err(port->dev, "deassert mgmt_sticky_rst err %d\n", err); >> + return err; >> + } >> + err = reset_control_deassert(port->pipe_rst); >> + if (err) { >> + dev_err(port->dev, "deassert pipe_rst err %d\n", err); >> + return err; >> + } >> + >> + pcie_write(port, 1 << 17 | 1 << 1, PCIE_CLIENT_BASE); >> + >> + gpiod_set_value(port->ep_gpio, 1); >> + err = -ETIMEDOUT; >> + while (time_before(jiffies, timeout)) { >> + status = pcie_read(port, PCIE_CLIENT_BASIC_STATUS1); >> + if (((status >> 20) & 0x3) == 0x3) { >> + dev_info(port->dev, "pcie link training gen1 pass!\n"); >> + err = 0; >> + break; >> + } >> + } >> + if (err) { >> + dev_err(port->dev, "pcie link training gen1 timeout!\n"); >> + return err; >> + } >> + >> + status = pcie_read(port, 0x9000d0); >> + status |= 0x20; >> + pcie_write(port, status, 0x9000d0); > > just to mention it again, bit handling as descriptive constant please and > also please give that register a name :-) will address this magic number all above using a new header file :) > > [...] > >> +static int rockchip_pcie_parse_dt(struct rockchip_pcie_port *port) >> +{ > > [...] > >> + port->lanes = 1; >> + err = of_property_read_u32(node, "num-lanes", &port->lanes); >> + if (!err && ((port->lanes == 0) || >> + (port->lanes == 3) || >> + (port->lanes > 4))) { >> + dev_info(dev, "invalid num-lanes, default use one lane\n"); > > dev_warn might be more appropriate sure. > >> + port->lanes = 1; >> + } > > [...] > >> +static void rockchip_pcie_msi_enable(struct rockchip_pcie_port *pp) >> +{ >> + struct device_node *msi_node; >> + >> + msi_node = of_parse_phandle(pp->dev->of_node, >> + "msi-parent", 0); > > I would assume this should live in the general parse_dt function? > Also is that MSI enablement really allow to fail silently without any affect > on the PCI functionality? yes, we should check this as well as CONFIG_PCI_MSI. Will fix it. > >> + if (!msi_node) >> + return; >> + >> + pp->msi = of_pci_find_msi_chip_by_node(msi_node); >> + of_node_put(msi_node); >> + >> + if (pp->msi) >> + pp->msi->dev = pp->dev; >> +} > > [...] > >> +static int rockchip_pcie_probe(struct platform_device *pdev) >> +{ >> + struct rockchip_pcie_port *port; >> + struct device *dev = &pdev->dev; >> + struct pci_bus *bus, *child; >> + struct resource_entry *win; >> + int reg_no; >> + int err = 0; >> + int irq; >> + LIST_HEAD(res); >> + >> + if (!dev->of_node) >> + return -ENODEV; >> + >> + port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL); >> + if (!port) >> + return -ENOMEM; >> + >> + irq = platform_get_irq_byname(pdev, "pcie-sys"); >> + if (irq < 0) { >> + dev_err(dev, "missing pcie_sys IRQ resource\n"); >> + return -EINVAL; >> + } > > blank line > >> + err = devm_request_irq(dev, irq, rockchip_pcie_subsys_irq_handler, >> + IRQF_SHARED, "pcie-sys", port); >> + if (err) { >> + dev_err(dev, "failed to request pcie subsystem irq\n"); >> + return err; >> + } >> + >> + port->irq = platform_get_irq_byname(pdev, "pcie-legacy"); >> + if (port->irq < 0) { >> + dev_err(dev, "missing pcie_legacy IRQ resource\n"); >> + return -EINVAL; >> + } > > blank line > >> + err = devm_request_irq(dev, port->irq, >> + rockchip_pcie_legacy_int_handler, >> + IRQF_SHARED, >> + "pcie-legacy", >> + port); >> + if (err) { >> + dev_err(&pdev->dev, "failed to request pcie-legacy irq\n"); >> + return err; >> + } >> + >> + irq = platform_get_irq_byname(pdev, "pcie-client"); >> + if (irq < 0) { >> + dev_err(dev, "missing pcie-client IRQ resource\n"); >> + return -EINVAL; >> + } > > blank line > >> + err = devm_request_irq(dev, irq, rockchip_pcie_client_irq_handler, >> + IRQF_SHARED, "pcie-client", port); >> + if (err) { >> + dev_err(dev, "failed to request pcie client irq\n"); >> + return err; >> + } >> + >> + port->dev = dev; >> + err = rockchip_pcie_parse_dt(port); >> + if (err) { >> + dev_err(dev, "Parsing DT failed\n"); >> + return err; >> + } > > rockchip_pcie_parse_dt already emits error messages that are a lot more > specific to the actual problem, so you don't need another error message here > and can just return the error code. > okay. > >> + err = rockchip_pcie_init_port(port); >> + if (err) >> + return err; >> + >> + platform_set_drvdata(pdev, port); >> + >> + rockchip_pcie_enable_interrupts(port); >> + if (!IS_ENABLED(CONFIG_PCI_MSI)) { >> + err = rockchip_pcie_init_irq_domain(port); >> + if (err < 0) >> + return err; >> + } >> + >> + err = of_pci_get_host_bridge_resources(dev->of_node, 0, 0xff, >> + &res, &port->io_base); >> + if (err) >> + return err; > > add blank line > > > Heiko > > >
On 2016/5/23 8:48, Shawn Lin wrote: > On 2016/5/21 5:13, Heiko Stuebner wrote: >> Hi Shawn, >> >> I haven't had any contact with PCI yet, so my comments below will likely >> address more generic findings only. >> >> As you might've guessed from the binding comments, to me it looks like >> the >> phy handling should be in a separate phy driver and looking below all phy >> accesses seem very separate from the actual pci controller interactions - >> they are even in port_init only as well. >> > > yes, the main reason for not to seperate a new pcie-phy driver for this > prototype design is that I just wonder whether it's worth to create a > new driver for just a small piece of code. And a bit more forward is > that I think phy api is no so scalable for just init/power_on in > case of too much interactions between controller and phy from which I > suffer a bit for emmc. > > Should I really need to seperate the phy part into pcie-phy? ;) Currently, We still need some more interactions between phy and controller here, not just what you point out in the comments. so if we need a seperate phy driver, could it allows me to export some functions for pcie driver? > >> While I already added some comments about that below, the driver seems >> full >> of raw register bit handling (including wild shifts). Please abstract >> that >> using contants, so that stuff stays readable for the future. > > okay, let me migrate these magic number and raw reg bit handling into a > new header file. > >> >> >> Am Freitag, 20. Mai 2016, 18:29:16 schrieb Shawn Lin: >>> RK3399 has a PCIe controller which can be used as Root Complex. >>> This driver supports a PCIe controller as Root Complex mode. >>> >>> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com> >>> --- >> >> [...] >> >>> diff --git a/drivers/pci/host/pcie-rockchip.c >>> b/drivers/pci/host/pcie-rockchip.c new file mode 100644 >>> index 0000000..4063fd3 >>> --- /dev/null >>> +++ b/drivers/pci/host/pcie-rockchip.c >>> @@ -0,0 +1,1181 @@ >>> +/* >>> + * Rockchip AXI PCIe host controller driver >>> + * >>> + * Copyright (c) 2016 Rockchip, Inc. >>> + * >>> + * Author: Shawn Lin <shawn.lin@rock-chips.com> >>> + * Wenrui Li <wenrui.li@rock-chips.com> >>> + * Bits taken from Synopsys Designware Host controller driver and >>> + * ARM PCI Host generic driver. >>> + * >>> + * This program is free software: you can redistribute it and/or modify >>> + * it under the terms of the GNU General Public License as published by >>> + * the Free Software Foundation, either version 2 of the License, or >>> + * (at your option) any later version. >>> + */ >>> +#include <linux/clk.h> >>> +#include <linux/delay.h> >>> +#include <linux/gpio/consumer.h> >>> +#include <linux/interrupt.h> >>> +#include <linux/irq.h> >>> +#include <linux/irqdomain.h> >>> +#include <linux/kernel.h> >>> +#include <linux/mfd/syscon.h> >>> +#include <linux/module.h> >>> +#include <linux/msi.h> >>> +#include <linux/of_address.h> >>> +#include <linux/of_device.h> >>> +#include <linux/of_pci.h> >>> +#include <linux/of_platform.h> >>> +#include <linux/of_irq.h> >>> +#include <linux/pci.h> >>> +#include <linux/platform_device.h> >>> +#include <linux/reset.h> >>> +#include <linux/regmap.h> >>> + >>> +#define REF_CLK_100MHZ (100 * 1000 * 1000) >> >> seems unused > > will be removed. > >> >>> +#define PCIE_CLIENT_BASE 0x0 >>> +#define PCIE_RC_CONFIG_BASE 0xa00000 >>> +#define PCIE_CORE_CTRL_MGMT_BASE 0x900000 >>> +#define PCIE_CORE_AXI_CONF_BASE 0xc00000 >>> +#define PCIE_CORE_AXI_INBOUND_BASE 0xc00800 >>> + >>> +#define PCIE_CLIENT_BASIC_STATUS0 0x44 >>> +#define PCIE_CLIENT_BASIC_STATUS1 0x48 >>> +#define PCIE_CLIENT_INT_MASK 0x4c >>> +#define PCIE_CLIENT_INT_STATUS 0x50 >>> +#define PCIE_CORE_INT_MASK 0x900210 >>> +#define PCIE_CORE_INT_STATUS 0x90020c >>> + >>> +/** Size of one AXI Region (not Region 0) */ >>> +#define AXI_REGION_SIZE (0x1 << 20) >> >> for those generic (1 << x) things please use BIT(x) instead >> Also constants intertwined with constants is hard to read, >> so ideally add a blank line above each comment and comment style for >> single >> line comments only has one * in the beginning > > Ahh yep. > >> /* foo */ >> >>> +/** Overall size of AXI area */ >>> +#define AXI_OVERALL_SIZE (64 * (0x1 << 20)) >>> +/** Size of Region 0, equal to sum of sizes of other regions */ >>> +#define AXI_REGION_0_SIZE (32 * (0x1 << 20)) >>> +#define OB_REG_SIZE_SHIFT 5 >>> +#define IB_ROOT_PORT_REG_SIZE_SHIFT 3 >>> + >>> +#define AXI_WRAPPER_IO_WRITE 0x6 >>> +#define AXI_WRAPPER_MEM_WRITE 0x2 >>> +#define MAX_AXI_IB_ROOTPORT_REGION_NUM 3 >>> +#define MIN_AXI_ADDR_BITS_PASSED 8 >> >> strange spacing after #define > > Generally I use one space after #define, but I donnot somehow... > I will check it twice. > >> >> [...] >> >>> +/** >>> + * rockchip_pcie_init_port - Initialize hardware >>> + * @port: PCIe port information >>> + */ >>> +static int rockchip_pcie_init_port(struct rockchip_pcie_port *port) >>> +{ >>> + int err; >>> + u32 status; >>> + unsigned long timeout = jiffies + msecs_to_jiffies(1000); >> >> this timeout only seems to be initialized once here but used for multiple >> loops below resulting in all waitloops combined together being allowed to >> take 1 second ... is this intended? > > a big timeout value to make sure we leave enough margin. No any data > is provided about how long should we wait for pll lock/re-lock/tainning > finish stuff. As it also related to the Socs/devices. Currently I > test pci-ethernet/wifi/SATA-bridge/USB-bridge, and it seems can be > reduced. But as you know, I cannot guarantee not to augment it once we > find some SSD/GPU in the failure state of trainning in the future. > Anyway I think here we use loop-break method, so it should be not > harmful? > >> >>> + gpiod_set_value(port->ep_gpio, 0); >>> + >>> + /* Make sure PCIe relate block is in reset state */ >>> + err = reset_control_assert(port->phy_rst); >>> + if (err) { >>> + dev_err(port->dev, "assert phy_rst err %d\n", err); >>> + return err; >>> + } >> >> should move to phy driver probe or so >> >>> + err = reset_control_assert(port->core_rst); >>> + if (err) { >>> + dev_err(port->dev, "assert core_rst err %d\n", err); >>> + return err; >>> + } >> >> blank line >> >>> + err = reset_control_assert(port->mgmt_rst); >>> + if (err) { >>> + dev_err(port->dev, "assert mgmt_rst err %d\n", err); >>> + return err; >>> + } >> >> blank line >> >>> + err = reset_control_assert(port->mgmt_sticky_rst); >>> + if (err) { >>> + dev_err(port->dev, "assert mgmt_sticky_rst err %d\n", err); >>> + return err; >>> + } >> >> blank line >> >>> + err = reset_control_assert(port->pipe_rst); >>> + if (err) { >>> + dev_err(port->dev, "assert pipe_rst err %d\n", err); >>> + return err; >>> + } >>> + >>> + pcie_write(port, (0xf << 20) | (0x1 << 16) | >>> PCIE_CLIENT_GEN_SEL_2 | >>> + (0x1 << 19) | (0x1 << 3) | >>> + PCIE_CLIENT_MODE_RC | >>> + PCIE_CLIENT_CONF_LANE_NUM(port->lanes) | >>> + PCIE_CLIENT_CONF_ENABLE, PCIE_CLIENT_BASE); >>> + >> >> ------ phy_enable begin ------ >> >> As mentioned above, the whole phy handling seems pretty separate, so >> should >> be easily being able to live in a real phy driver? >> >> This of course also needs to handle the phy clock in it. >> >>> + err = reset_control_deassert(port->phy_rst); >>> + if (err) { >>> + dev_err(port->dev, "deassert phy_rst err %d\n", err); >>> + return err; >>> + } >>> + regmap_write(port->grf, port->pcie_conf, >>> + (0x3f << 17) | (0x10 << 1)); >> >> the bits above should use some sort of constant / description. Also see >> HIWORD_UPDATE in other drivers for the write-enable mask >> >> also add blank line here >> >>> + err = -EINVAL; >>> + while (time_before(jiffies, timeout)) { >>> + regmap_read(port->grf, port->pcie_status, &status); >>> + if ((status & (1 << 9))) { >> >> use a constant for the (1 << 9) [aka BIT(9)] please >> >>> + dev_info(port->dev, "pll locked!\n"); >> >> dev_dbg instead? > > yep > >> >>> + err = 0; >>> + break; >>> + } >>> + } >>> + if (err) { >>> + dev_err(port->dev, "pll lock timeout!\n"); >>> + return err; >>> + } >>> + pcie_pb_wr_cfg(port, 0x10, 0x8); >>> + pcie_pb_wr_cfg(port, 0x12, 0x8); >>> + >>> + err = -ETIMEDOUT; >>> + while (time_before(jiffies, timeout)) { >>> + regmap_read(port->grf, port->pcie_status, &status); >>> + if (!(status & (1 << 10))) { >> >> constant again >> >>> + dev_info(port->dev, "pll output enable done!\n"); >> >> dev_dbg or leave it out > > dev_dbg should be fine. > >> >>> + err = 0; >>> + break; >>> + } >>> + } >>> + >>> + if (err) { >>> + dev_err(port->dev, "pll output enable timeout!\n"); >>> + return err; >>> + } >>> + >>> + regmap_write(port->grf, port->pcie_conf, >>> + (0x3f << 17) | (0x10 << 1)); >>> + err = -EINVAL; >>> + while (time_before(jiffies, timeout)) { >>> + regmap_read(port->grf, port->pcie_status, &status); >>> + if ((status & (1 << 9))) { >>> + dev_info(port->dev, "pll relocked!\n"); >>> + err = 0; >>> + break; >>> + } >>> + } >>> + if (err) { >>> + dev_err(port->dev, "pll relock timeout!\n"); >>> + return err; >>> + } >> >> ------ phy_enable end ------ >> >> >>> + err = reset_control_deassert(port->core_rst); >>> + if (err) { >>> + dev_err(port->dev, "deassert core_rst err %d\n", err); >>> + return err; >>> + } >>> + err = reset_control_deassert(port->mgmt_rst); >>> + if (err) { >>> + dev_err(port->dev, "deassert mgmt_rst err %d\n", err); >>> + return err; >>> + } >>> + err = reset_control_deassert(port->mgmt_sticky_rst); >>> + if (err) { >>> + dev_err(port->dev, "deassert mgmt_sticky_rst err %d\n", err); >>> + return err; >>> + } >>> + err = reset_control_deassert(port->pipe_rst); >>> + if (err) { >>> + dev_err(port->dev, "deassert pipe_rst err %d\n", err); >>> + return err; >>> + } >>> + >>> + pcie_write(port, 1 << 17 | 1 << 1, PCIE_CLIENT_BASE); >>> + >>> + gpiod_set_value(port->ep_gpio, 1); >>> + err = -ETIMEDOUT; >>> + while (time_before(jiffies, timeout)) { >>> + status = pcie_read(port, PCIE_CLIENT_BASIC_STATUS1); >>> + if (((status >> 20) & 0x3) == 0x3) { >>> + dev_info(port->dev, "pcie link training gen1 pass!\n"); >>> + err = 0; >>> + break; >>> + } >>> + } >>> + if (err) { >>> + dev_err(port->dev, "pcie link training gen1 timeout!\n"); >>> + return err; >>> + } >>> + >>> + status = pcie_read(port, 0x9000d0); >>> + status |= 0x20; >>> + pcie_write(port, status, 0x9000d0); >> >> just to mention it again, bit handling as descriptive constant please and >> also please give that register a name :-) > > will address this magic number all above using a new header file :) > >> >> [...] >> >>> +static int rockchip_pcie_parse_dt(struct rockchip_pcie_port *port) >>> +{ >> >> [...] >> >>> + port->lanes = 1; >>> + err = of_property_read_u32(node, "num-lanes", &port->lanes); >>> + if (!err && ((port->lanes == 0) || >>> + (port->lanes == 3) || >>> + (port->lanes > 4))) { >>> + dev_info(dev, "invalid num-lanes, default use one lane\n"); >> >> dev_warn might be more appropriate > > sure. > >> >>> + port->lanes = 1; >>> + } >> >> [...] >> >>> +static void rockchip_pcie_msi_enable(struct rockchip_pcie_port *pp) >>> +{ >>> + struct device_node *msi_node; >>> + >>> + msi_node = of_parse_phandle(pp->dev->of_node, >>> + "msi-parent", 0); >> >> I would assume this should live in the general parse_dt function? >> Also is that MSI enablement really allow to fail silently without any >> affect >> on the PCI functionality? > > yes, we should check this as well as CONFIG_PCI_MSI. > Will fix it. > > >> >>> + if (!msi_node) >>> + return; >>> + >>> + pp->msi = of_pci_find_msi_chip_by_node(msi_node); >>> + of_node_put(msi_node); >>> + >>> + if (pp->msi) >>> + pp->msi->dev = pp->dev; >>> +} >> >> [...] >> >>> +static int rockchip_pcie_probe(struct platform_device *pdev) >>> +{ >>> + struct rockchip_pcie_port *port; >>> + struct device *dev = &pdev->dev; >>> + struct pci_bus *bus, *child; >>> + struct resource_entry *win; >>> + int reg_no; >>> + int err = 0; >>> + int irq; >>> + LIST_HEAD(res); >>> + >>> + if (!dev->of_node) >>> + return -ENODEV; >>> + >>> + port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL); >>> + if (!port) >>> + return -ENOMEM; >>> + >>> + irq = platform_get_irq_byname(pdev, "pcie-sys"); >>> + if (irq < 0) { >>> + dev_err(dev, "missing pcie_sys IRQ resource\n"); >>> + return -EINVAL; >>> + } >> >> blank line >> >>> + err = devm_request_irq(dev, irq, rockchip_pcie_subsys_irq_handler, >>> + IRQF_SHARED, "pcie-sys", port); >>> + if (err) { >>> + dev_err(dev, "failed to request pcie subsystem irq\n"); >>> + return err; >>> + } >>> + >>> + port->irq = platform_get_irq_byname(pdev, "pcie-legacy"); >>> + if (port->irq < 0) { >>> + dev_err(dev, "missing pcie_legacy IRQ resource\n"); >>> + return -EINVAL; >>> + } >> >> blank line >> >>> + err = devm_request_irq(dev, port->irq, >>> + rockchip_pcie_legacy_int_handler, >>> + IRQF_SHARED, >>> + "pcie-legacy", >>> + port); >>> + if (err) { >>> + dev_err(&pdev->dev, "failed to request pcie-legacy irq\n"); >>> + return err; >>> + } >>> + >>> + irq = platform_get_irq_byname(pdev, "pcie-client"); >>> + if (irq < 0) { >>> + dev_err(dev, "missing pcie-client IRQ resource\n"); >>> + return -EINVAL; >>> + } >> >> blank line >> >>> + err = devm_request_irq(dev, irq, rockchip_pcie_client_irq_handler, >>> + IRQF_SHARED, "pcie-client", port); >>> + if (err) { >>> + dev_err(dev, "failed to request pcie client irq\n"); >>> + return err; >>> + } >>> + >>> + port->dev = dev; >>> + err = rockchip_pcie_parse_dt(port); >>> + if (err) { >>> + dev_err(dev, "Parsing DT failed\n"); >>> + return err; >>> + } >> >> rockchip_pcie_parse_dt already emits error messages that are a lot more >> specific to the actual problem, so you don't need another error >> message here >> and can just return the error code. >> > > okay. > >> >>> + err = rockchip_pcie_init_port(port); >>> + if (err) >>> + return err; >>> + >>> + platform_set_drvdata(pdev, port); >>> + >>> + rockchip_pcie_enable_interrupts(port); >>> + if (!IS_ENABLED(CONFIG_PCI_MSI)) { >>> + err = rockchip_pcie_init_irq_domain(port); >>> + if (err < 0) >>> + return err; >>> + } >>> + >>> + err = of_pci_get_host_bridge_resources(dev->of_node, 0, 0xff, >>> + &res, &port->io_base); >>> + if (err) >>> + return err; >> >> add blank line >> >> >> Heiko >> >> >> > >
> + > + irq = platform_get_irq_byname(pdev, "pcie-sys"); > + if (irq < 0) { > + dev_err(dev, "missing pcie_sys IRQ resource\n"); > + return -EINVAL; > + } > + err = devm_request_irq(dev, irq, rockchip_pcie_subsys_irq_handler, > + IRQF_SHARED, "pcie-sys", port); > + if (err) { > + dev_err(dev, "failed to request pcie subsystem irq\n"); > + return err; > + } > + > + port->irq = platform_get_irq_byname(pdev, "pcie-legacy"); > + if (port->irq < 0) { > + dev_err(dev, "missing pcie_legacy IRQ resource\n"); > + return -EINVAL; > + } > + err = devm_request_irq(dev, port->irq, > + rockchip_pcie_legacy_int_handler, > + IRQF_SHARED, > + "pcie-legacy", > + port); > + if (err) { > + dev_err(&pdev->dev, "failed to request pcie-legacy irq\n"); > + return err; > + } > + > + irq = platform_get_irq_byname(pdev, "pcie-client"); > + if (irq < 0) { > + dev_err(dev, "missing pcie-client IRQ resource\n"); > + return -EINVAL; > + } > + err = devm_request_irq(dev, irq, rockchip_pcie_client_irq_handler, > + IRQF_SHARED, "pcie-client", port); > + if (err) { > + dev_err(dev, "failed to request pcie client irq\n"); > + return err; > + } > + All of the above interrupt number details are being obtained from device tree, why not move above API's to rockchip_pcie_parse_dt. > + port->dev = dev; > + err = rockchip_pcie_parse_dt(port); > + if (err) { > + dev_err(dev, "Parsing DT failed\n"); > + return err; > + } > + > + pcie_write(port, 0x6040000, PCIE_RC_CONFIG_BASE + 0x8); > + pcie_write(port, 0x0, PCIE_CORE_CTRL_MGMT_BASE + 0x300); > + > + pcie_write(port, (RC_REGION_0_ADDR_TRANS_L + > RC_REGION_0_PASS_BITS), > + PCIE_CORE_AXI_CONF_BASE); > + pcie_write(port, RC_REGION_0_ADDR_TRANS_H, > + PCIE_CORE_AXI_CONF_BASE + 0x4); > + pcie_write(port, 0x0080000a, PCIE_CORE_AXI_CONF_BASE + 0x8); > + pcie_write(port, 0x00000000, PCIE_CORE_AXI_CONF_BASE + 0xc); Why not move the above steps to rockchip_pcie_init_port function ? Please use macros instead of using values. -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Bharat, On 2016/5/23 23:15, Bharat Kumar Gogada wrote: >> + >> + irq = platform_get_irq_byname(pdev, "pcie-sys"); >> + if (irq < 0) { >> + dev_err(dev, "missing pcie_sys IRQ resource\n"); >> + return -EINVAL; >> + } >> + err = devm_request_irq(dev, irq, rockchip_pcie_subsys_irq_handler, >> + IRQF_SHARED, "pcie-sys", port); >> + if (err) { >> + dev_err(dev, "failed to request pcie subsystem irq\n"); >> + return err; >> + } >> + >> + port->irq = platform_get_irq_byname(pdev, "pcie-legacy"); >> + if (port->irq < 0) { >> + dev_err(dev, "missing pcie_legacy IRQ resource\n"); >> + return -EINVAL; >> + } >> + err = devm_request_irq(dev, port->irq, >> + rockchip_pcie_legacy_int_handler, >> + IRQF_SHARED, >> + "pcie-legacy", >> + port); >> + if (err) { >> + dev_err(&pdev->dev, "failed to request pcie-legacy irq\n"); >> + return err; >> + } >> + >> + irq = platform_get_irq_byname(pdev, "pcie-client"); >> + if (irq < 0) { >> + dev_err(dev, "missing pcie-client IRQ resource\n"); >> + return -EINVAL; >> + } >> + err = devm_request_irq(dev, irq, rockchip_pcie_client_irq_handler, >> + IRQF_SHARED, "pcie-client", port); >> + if (err) { >> + dev_err(dev, "failed to request pcie client irq\n"); >> + return err; >> + } >> + > > All of the above interrupt number details are being obtained from device tree, why not move above API's to rockchip_pcie_parse_dt. Ok, I will move these above into parse_dt function. > >> + port->dev = dev; >> + err = rockchip_pcie_parse_dt(port); >> + if (err) { >> + dev_err(dev, "Parsing DT failed\n"); >> + return err; >> + } >> + >> + pcie_write(port, 0x6040000, PCIE_RC_CONFIG_BASE + 0x8); >> + pcie_write(port, 0x0, PCIE_CORE_CTRL_MGMT_BASE + 0x300); >> + >> + pcie_write(port, (RC_REGION_0_ADDR_TRANS_L + >> RC_REGION_0_PASS_BITS), >> + PCIE_CORE_AXI_CONF_BASE); >> + pcie_write(port, RC_REGION_0_ADDR_TRANS_H, >> + PCIE_CORE_AXI_CONF_BASE + 0x4); >> + pcie_write(port, 0x0080000a, PCIE_CORE_AXI_CONF_BASE + 0x8); >> + pcie_write(port, 0x00000000, PCIE_CORE_AXI_CONF_BASE + 0xc); > > Why not move the above steps to rockchip_pcie_init_port function ? > Please use macros instead of using values. okay, will improve them. Thanks for comment. > > >
On Friday, May 20, 2016 6:29:16 PM CEST Shawn Lin wrote: > +static int rockchip_pcie_wr_own_conf(struct rockchip_pcie_port *pp, > + int where, int size, u32 val) > +{ > + u32 tmp; > + int offset; > + > + offset = (where & (~0x3)); > + tmp = readl(pp->apb_base + PCIE_RC_CONFIG_BASE + offset); > + if (size == 4) { > + writel(val, pp->apb_base + PCIE_RC_CONFIG_BASE + where); > + } else if (size == 2) { > + if (where & 0x2) > + tmp = ((tmp & 0xffff) | (val << 16)); > + else > + tmp = ((tmp & 0xffff0000) | val); > + > + writel(tmp, pp->apb_base + PCIE_RC_CONFIG_BASE + offset); > + } else if (size == 1) { > + if ((where & 0x3) == 0) > + tmp = ((tmp & (~0xff)) | val); > + else if ((where & 0x3) == 1) > + tmp = ((tmp & (~0xff00)) | (val << 8)); > + else if ((where & 0x3) == 2) > + tmp = ((tmp & (~0xff0000)) | (val << 16)); > + else if ((where & 0x3) == 3) > + tmp = ((tmp & (~0xff000000)) | (val << 24)); > + > + writel(tmp, pp->apb_base + PCIE_RC_CONFIG_BASE + offset); > + } else { > + return PCIBIOS_BAD_REGISTER_NUMBER; > + } > + return PCIBIOS_SUCCESSFUL; > +} Why can't you access the individual sub-word registers here? > + > +static int rockchip_pcie_rd_other_conf(struct rockchip_pcie_port *pp, > + struct pci_bus *bus, u32 devfn, > + int where, int size, u32 *val) > +{ > + u32 busdev; > + > + busdev = PCIE_ECAM_ADDR(bus->number, PCI_SLOT(devfn), > + PCI_FUNC(devfn), where); > + > + if (busdev & (size - 1)) { > + *val = 0; > + return PCIBIOS_BAD_REGISTER_NUMBER; > + } > + > + if (size == 4) { > + *val = readl(pp->reg_base + busdev); > + } else if (size == 2) { > + *val = readw(pp->reg_base + busdev); > + } else if (size == 1) { > + *val = readb(pp->reg_base + busdev); > + } else { > + *val = 0; > + return PCIBIOS_BAD_REGISTER_NUMBER; > + } > + return PCIBIOS_SUCCESSFUL; > +} > + This looks like the normal ECAM operations, you could just call those. > + while (time_before(jiffies, timeout)) { > + regmap_read(port->grf, port->pcie_status, &status); > + if ((status & (1 << 9))) { > + dev_info(port->dev, "pll locked!\n"); > + err = 0; > + break; > + } > + } Maybe add an msleep(1) here to avoid busy-looping? > + for (reg_no = 0; reg_no < (port->mem_size >> 20); reg_no++) { > + err = rockchip_pcie_prog_ob_atu(port, reg_no + 1, > + AXI_WRAPPER_MEM_WRITE, > + 20 - 1, > + port->mem_bus_addr + > + (reg_no << 20), > + 0); > + if (err) { > + dev_err(dev, "Program RC outbound atu failed\n"); > + return err; > + } > + } What if there is more than one outbound memory window, e.g. prefetchable and non-prefetchable? Where do you set the I/O window? > + err = rockchip_pcie_prog_ib_atu(port, 2, 32 - 1, 0x0, 0); > + if (err) { > + dev_err(dev, "Program RC inbound atu failed\n"); > + return err; > + } And this doesn't seem to reflect the DMA ranges. > + > + port->root_bus_nr = port->busn->start; > + if (IS_ENABLED(CONFIG_PCI_MSI)) { > + bus = pci_scan_root_bus_msi(port->dev, port->root_bus_nr, > + &rockchip_pcie_ops, port, &res, > + port->msi); > + } else { > + bus = pci_scan_root_bus(&pdev->dev, 0, > + &rockchip_pcie_ops, port, &res); PCI_MSI is selected unconditionally from Kconfig for this driver, so no need for the compile-time check here. > + > +static int rockchip_pcie_remove(struct platform_device *pdev) > +{ > + struct rockchip_pcie_port *port = platform_get_drvdata(pdev); > + > + clk_disable_unprepare(port->hclk_pcie); > + clk_disable_unprepare(port->aclk_perf_pcie); > + clk_disable_unprepare(port->aclk_pcie); > + clk_disable_unprepare(port->clk_pciephy_ref); > + > + return 0; > +} You don't seem to remove the child devices here. Have you tried unloading the module? Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> RK3399 has a PCIe controller which can be used as Root Complex. > This driver supports a PCIe controller as Root Complex mode. 1 general feedback I have is that there are a lot of magic constants. I'd appreciate if they could be converted into macros for easy readability. > > Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com> > --- > > drivers/pci/host/Kconfig | 12 + > drivers/pci/host/Makefile | 1 + > drivers/pci/host/pcie-rockchip.c | 1181 ++++++++++++++++++++++++++++++++++++++ > 3 files changed, 1194 insertions(+) > create mode 100644 drivers/pci/host/pcie-rockchip.c > > diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig > index 8e4f038..76447a8 100644 > --- a/drivers/pci/host/Kconfig > +++ b/drivers/pci/host/Kconfig > @@ -245,4 +245,16 @@ config PCIE_ARMADA_8K > Designware hardware and therefore the driver re-uses the > Designware core functions to implement the driver. > > +config PCIE_ROCKCHIP > + bool "Rockchip PCIe controller" > + depends on ARM64 && ARCH_ROCKCHIP > + depends on OF > + select MFD_SYSCON > + select PCI_MSI > + select PCI_MSI_IRQ_DOMAIN > + help > + Say Y here if you want internal PCI support on Rockchip SoC. > + There are 1 internal PCIe port available to support GEN2 with > + 4 slots. > + > endmenu > diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile > index a6f85e3..fb3871e 100644 > --- a/drivers/pci/host/Makefile > +++ b/drivers/pci/host/Makefile > @@ -29,3 +29,4 @@ obj-$(CONFIG_PCIE_QCOM) += pcie-qcom.o > obj-$(CONFIG_PCI_HOST_THUNDER_ECAM) += pci-thunder-ecam.o > obj-$(CONFIG_PCI_HOST_THUNDER_PEM) += pci-thunder-pem.o > obj-$(CONFIG_PCIE_ARMADA_8K) += pcie-armada8k.o > +obj-$(CONFIG_PCIE_ROCKCHIP) += pcie-rockchip.o > diff --git a/drivers/pci/host/pcie-rockchip.c b/drivers/pci/host/pcie-rockchip.c > new file mode 100644 > index 0000000..4063fd3 > --- /dev/null > +++ b/drivers/pci/host/pcie-rockchip.c > @@ -0,0 +1,1181 @@ > +/* > + * Rockchip AXI PCIe host controller driver > + * > + * Copyright (c) 2016 Rockchip, Inc. > + * > + * Author: Shawn Lin <shawn.lin@rock-chips.com> > + * Wenrui Li <wenrui.li@rock-chips.com> > + * Bits taken from Synopsys Designware Host controller driver and > + * ARM PCI Host generic driver. > + * > + * This program is free software: you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation, either version 2 of the License, or > + * (at your option) any later version. > + */ > +#include <linux/clk.h> > +#include <linux/delay.h> > +#include <linux/gpio/consumer.h> > +#include <linux/interrupt.h> > +#include <linux/irq.h> > +#include <linux/irqdomain.h> > +#include <linux/kernel.h> > +#include <linux/mfd/syscon.h> > +#include <linux/module.h> > +#include <linux/msi.h> > +#include <linux/of_address.h> > +#include <linux/of_device.h> > +#include <linux/of_pci.h> > +#include <linux/of_platform.h> > +#include <linux/of_irq.h> > +#include <linux/pci.h> > +#include <linux/platform_device.h> > +#include <linux/reset.h> > +#include <linux/regmap.h> > + > +#define REF_CLK_100MHZ (100 * 1000 * 1000) > +#define PCIE_CLIENT_BASE 0x0 > +#define PCIE_RC_CONFIG_BASE 0xa00000 > +#define PCIE_CORE_CTRL_MGMT_BASE 0x900000 > +#define PCIE_CORE_AXI_CONF_BASE 0xc00000 > +#define PCIE_CORE_AXI_INBOUND_BASE 0xc00800 > + > +#define PCIE_CLIENT_BASIC_STATUS0 0x44 > +#define PCIE_CLIENT_BASIC_STATUS1 0x48 > +#define PCIE_CLIENT_INT_MASK 0x4c > +#define PCIE_CLIENT_INT_STATUS 0x50 > +#define PCIE_CORE_INT_MASK 0x900210 > +#define PCIE_CORE_INT_STATUS 0x90020c > + > +/** Size of one AXI Region (not Region 0) */ > +#define AXI_REGION_SIZE (0x1 << 20) > +/** Overall size of AXI area */ > +#define AXI_OVERALL_SIZE (64 * (0x1 << 20)) Not used? > +/** Size of Region 0, equal to sum of sizes of other regions */ > +#define AXI_REGION_0_SIZE (32 * (0x1 << 20)) > +#define OB_REG_SIZE_SHIFT 5 > +#define IB_ROOT_PORT_REG_SIZE_SHIFT 3 > + > +#define AXI_WRAPPER_IO_WRITE 0x6 > +#define AXI_WRAPPER_MEM_WRITE 0x2 > +#define MAX_AXI_IB_ROOTPORT_REGION_NUM 3 > +#define MIN_AXI_ADDR_BITS_PASSED 8 > + > +#define ROCKCHIP_PCIE_RPIFR1_INTR_MASK GENMASK(8, 5) > +#define ROCKCHIP_PCIE_RPIFR1_INTR_SHIFT 5 > +#define CLIENT_INTERRUPTS \ > + (LOC_INT | INTA | INTB | INTC | INTD |\ > + CORR_ERR | NFATAL_ERR | FATAL_ERR | DPA_INT | \ > + HOT_RESET | MSG_DONE | LEGACY_DONE) > +#define CORE_INTERRUPTS \ > + (PRFPE | CRFPE | RRPE | CRFO | RT | RTR | \ > + PE | MTR | UCR | FCE | CT | UTC | MMVC) > +#define PWR_STCG BIT(0) > +#define HOT_PLUG BIT(1) > +#define PHY_INT BIT(2) > +#define UDMA_INT BIT(3) > +#define LOC_INT BIT(4) > +#define INTA BIT(5) > +#define INTB BIT(6) > +#define INTC BIT(7) > +#define INTD BIT(8) > +#define CORR_ERR BIT(9) > +#define NFATAL_ERR BIT(10) > +#define FATAL_ERR BIT(11) > +#define DPA_INT BIT(12) > +#define HOT_RESET BIT(13) > +#define MSG_DONE BIT(14) > +#define LEGACY_DONE BIT(15) > +#define PRFPE BIT(0) > +#define CRFPE BIT(1) > +#define RRPE BIT(2) > +#define PRFO BIT(3) > +#define CRFO BIT(4) > +#define RT BIT(5) > +#define RTR BIT(6) > +#define PE BIT(7) > +#define MTR BIT(8) > +#define UCR BIT(9) > +#define FCE BIT(10) > +#define CT BIT(11) > +#define UTC BIT(18) > +#define MMVC BIT(19) > + > +#define PCIE_ECAM_BUS(x) (((x) & 0xFF) << 20) > +#define PCIE_ECAM_DEV(x) (((x) & 0x1F) << 15) > +#define PCIE_ECAM_FUNC(x) (((x) & 0x7) << 12) > +#define PCIE_ECAM_REG(x) (((x) & 0xFFF) << 0) > +#define PCIE_ECAM_ADDR(bus, dev, func, reg) \ > + (PCIE_ECAM_BUS(bus) | PCIE_ECAM_DEV(dev) | \ > + PCIE_ECAM_FUNC(func) | PCIE_ECAM_REG(reg)) > + > +#define RC_REGION_0_ADDR_TRANS_H 0x00000000 > +#define RC_REGION_0_ADDR_TRANS_L 0x00000000 > +#define RC_REGION_0_PASS_BITS (25 - 1) > +#define RC_REGION_1_ADDR_TRANS_H 0x00000000 > +#define RC_REGION_1_ADDR_TRANS_L 0x00400000 > +#define RC_REGION_1_PASS_BITS (20 - 1) > +#define MAX_AXI_WRAPPER_REGION_NUM 33 > +#define PCIE_CLIENT_CONF_ENABLE BIT(0) > +#define PCIE_CLIENT_CONF_LANE_NUM(x) ((x / 2) << 4) > +#define PCIE_CLIENT_MODE_RC BIT(6) > +#define PCIE_CLIENT_GEN_SEL_2 BIT(7) > +#define PCIE_CLIENT_GEN_SEL_1 0x0 > + > +struct rockchip_pcie_port { > + void __iomem *reg_base; > + void __iomem *apb_base; > + struct regmap *grf; > + unsigned int pcie_conf; > + unsigned int pcie_status; > + unsigned int pcie_laneoff; > + struct reset_control *phy_rst; > + struct reset_control *core_rst; > + struct reset_control *mgmt_rst; > + struct reset_control *mgmt_sticky_rst; > + struct reset_control *pipe_rst; > + struct clk *aclk_pcie; > + struct clk *aclk_perf_pcie; > + struct clk *hclk_pcie; > + struct clk *clk_pciephy_ref; > + struct gpio_desc *ep_gpio; > + u32 lanes; > + resource_size_t io_base; > + struct resource *cfg; > + struct resource *io; > + struct resource *mem; Nit: It seems to me that these resource fields (cfg/io/mem) aren't really used, except for assignment and as temporary variables, so possibly can be get rid of, (unless you intend to use them in later patches, if any) > + struct resource *busn; > + phys_addr_t io_bus_addr; > + u32 io_size; > + phys_addr_t mem_bus_addr; > + u32 mem_size; > + u8 root_bus_nr; > + int irq; > + struct msi_controller *msi; > + > + struct device *dev; > + struct irq_domain *irq_domain; > +}; > + > +static inline u32 pcie_read(struct rockchip_pcie_port *port, u32 reg) > +{ > + return readl(port->apb_base + reg); > +} > + > +static inline void pcie_write(struct rockchip_pcie_port *port, > + u32 val, u32 reg) > +{ > + writel(val, port->apb_base + reg); > +} > + > +static inline void pcie_pb_wr_cfg(struct rockchip_pcie_port *port, > + u32 addr, u32 data) > +{ > + regmap_write(port->grf, port->pcie_conf, > + (0x3ff << 17) | (data << 7) | (addr << 1)); > + udelay(1); > + regmap_write(port->grf, port->pcie_conf, > + (0x1 << 16) | (0x1 << 0)); > + udelay(1); > + regmap_write(port->grf, port->pcie_conf, > + (0x1 << 16) | (0x0 << 0)); > +} > + > +static inline u32 pcie_pb_rd_cfg(struct rockchip_pcie_port *port, > + u32 addr) > +{ > + u32 val; > + > + regmap_write(port->grf, port->pcie_conf, > + (0x3ff << 17) | (addr << 1)); Taking cue from pcie_pb_wr_cfg(), I'm wondering if there is any delay required here? > + regmap_read(port->grf, port->pcie_status, &val); > + return val; > +} > + > +static int rockchip_pcie_valid_config(struct rockchip_pcie_port *pp, > + struct pci_bus *bus, int dev) > +{ > + /* access only one slot on each root port */ > + if (bus->number == pp->root_bus_nr && dev > 0) > + return 0; > + > + /* > + * do not read more than one device on the bus directly attached > + * to RC's (Virtual Bridge's) DS side. > + */ > + if (bus->primary == pp->root_bus_nr && dev > 0) > + return 0; > + > + return 1; > +} > + > +static int rockchip_pcie_rd_own_conf(struct rockchip_pcie_port *pp, > + int where, int size, > + u32 *val) > +{ > + void __iomem *addr = pp->apb_base + PCIE_RC_CONFIG_BASE + where; > + > + if ((uintptr_t)addr & (size - 1)) { Use IS_ALIGNED()? > + *val = 0; > + return PCIBIOS_BAD_REGISTER_NUMBER; > + } > + > + if (size == 4) { > + *val = readl(addr); > + } else if (size == 2) { > + *val = readw(addr); > + } else if (size == 1) { > + *val = readb(addr); > + } else { > + *val = 0; > + return PCIBIOS_BAD_REGISTER_NUMBER; > + } > + return PCIBIOS_SUCCESSFUL; > +} > + > +static int rockchip_pcie_wr_own_conf(struct rockchip_pcie_port *pp, > + int where, int size, u32 val) > +{ > + u32 tmp; > + int offset; > + > + offset = (where & (~0x3)); > + tmp = readl(pp->apb_base + PCIE_RC_CONFIG_BASE + offset); > + if (size == 4) { > + writel(val, pp->apb_base + PCIE_RC_CONFIG_BASE + where); > + } else if (size == 2) { > + if (where & 0x2) > + tmp = ((tmp & 0xffff) | (val << 16)); > + else > + tmp = ((tmp & 0xffff0000) | val); > + > + writel(tmp, pp->apb_base + PCIE_RC_CONFIG_BASE + offset); > + } else if (size == 1) { > + if ((where & 0x3) == 0) > + tmp = ((tmp & (~0xff)) | val); > + else if ((where & 0x3) == 1) > + tmp = ((tmp & (~0xff00)) | (val << 8)); > + else if ((where & 0x3) == 2) > + tmp = ((tmp & (~0xff0000)) | (val << 16)); > + else if ((where & 0x3) == 3) > + tmp = ((tmp & (~0xff000000)) | (val << 24)); > + > + writel(tmp, pp->apb_base + PCIE_RC_CONFIG_BASE + offset); Why not register sized writes instead of writels? > + } else { > + return PCIBIOS_BAD_REGISTER_NUMBER; > + } > + return PCIBIOS_SUCCESSFUL; > +} > + > +static int rockchip_pcie_rd_other_conf(struct rockchip_pcie_port *pp, > + struct pci_bus *bus, u32 devfn, > + int where, int size, u32 *val) > +{ > + u32 busdev; > + > + busdev = PCIE_ECAM_ADDR(bus->number, PCI_SLOT(devfn), > + PCI_FUNC(devfn), where); > + > + if (busdev & (size - 1)) { Use IS_ALIGNED() for here and the other occurences in above and below functions. > + *val = 0; > + return PCIBIOS_BAD_REGISTER_NUMBER; > + } > + > + if (size == 4) { > + *val = readl(pp->reg_base + busdev); > + } else if (size == 2) { > + *val = readw(pp->reg_base + busdev); > + } else if (size == 1) { > + *val = readb(pp->reg_base + busdev); > + } else { > + *val = 0; > + return PCIBIOS_BAD_REGISTER_NUMBER; > + } > + return PCIBIOS_SUCCESSFUL; > +} > + > +static int rockchip_pcie_wr_other_conf(struct rockchip_pcie_port *pp, > + struct pci_bus *bus, u32 devfn, > + int where, int size, u32 val) > +{ > + u32 busdev; > + > + busdev = PCIE_ECAM_ADDR(bus->number, PCI_SLOT(devfn), > + PCI_FUNC(devfn), where); > + if (busdev & (size - 1)) > + return PCIBIOS_BAD_REGISTER_NUMBER; > + > + if (size == 4) > + writel(val, pp->reg_base + busdev); > + else if (size == 2) > + writew(val, pp->reg_base + busdev); > + else if (size == 1) > + writeb(val, pp->reg_base + busdev); > + else > + return PCIBIOS_BAD_REGISTER_NUMBER; > + > + return PCIBIOS_SUCCESSFUL; > +} > + > +static int rockchip_pcie_rd_conf(struct pci_bus *bus, u32 devfn, int where, > + int size, u32 *val) > +{ > + struct rockchip_pcie_port *pp = bus->sysdata; > + int ret; > + > + if (rockchip_pcie_valid_config(pp, bus, PCI_SLOT(devfn)) == 0) { > + *val = 0xffffffff; > + return PCIBIOS_DEVICE_NOT_FOUND; > + } > + > + if (bus->number != pp->root_bus_nr) > + ret = rockchip_pcie_rd_other_conf(pp, bus, devfn, > + where, size, val); > + else > + ret = rockchip_pcie_rd_own_conf(pp, where, size, val); > + > + return ret; > +} > + > +static int rockchip_pcie_wr_conf(struct pci_bus *bus, u32 devfn, > + int where, int size, u32 val) > +{ > + struct rockchip_pcie_port *pp = bus->sysdata; > + int ret; > + > + if (rockchip_pcie_valid_config(pp, bus, PCI_SLOT(devfn)) == 0) > + return PCIBIOS_DEVICE_NOT_FOUND; > + > + if (bus->number != pp->root_bus_nr) > + ret = rockchip_pcie_wr_other_conf(pp, bus, devfn, > + where, size, val); > + else > + ret = rockchip_pcie_wr_own_conf(pp, where, size, val); > + > + return ret; > +} > + > +static struct pci_ops rockchip_pcie_ops = { > + .read = rockchip_pcie_rd_conf, > + .write = rockchip_pcie_wr_conf, > +}; > + > +/** > + * rockchip_pcie_init_port - Initialize hardware > + * @port: PCIe port information > + */ > +static int rockchip_pcie_init_port(struct rockchip_pcie_port *port) > +{ > + int err; > + u32 status; > + unsigned long timeout = jiffies + msecs_to_jiffies(1000); The way you have currently implemented, would mean that you have set the cumulative timeout of 1000 msec for all the operations. So if PLL takes 990 msec to lock, there is only 10 msec for other steps. It may be better to have individual timeouts I think since you'd know from HW docs how much should the time out should be? > + > + gpiod_set_value(port->ep_gpio, 0); > + > + /* Make sure PCIe relate block is in reset state */ > + err = reset_control_assert(port->phy_rst); > + if (err) { > + dev_err(port->dev, "assert phy_rst err %d\n", err); > + return err; > + } > + err = reset_control_assert(port->core_rst); > + if (err) { > + dev_err(port->dev, "assert core_rst err %d\n", err); > + return err; > + } > + err = reset_control_assert(port->mgmt_rst); > + if (err) { > + dev_err(port->dev, "assert mgmt_rst err %d\n", err); > + return err; > + } > + err = reset_control_assert(port->mgmt_sticky_rst); > + if (err) { > + dev_err(port->dev, "assert mgmt_sticky_rst err %d\n", err); > + return err; > + } > + err = reset_control_assert(port->pipe_rst); > + if (err) { > + dev_err(port->dev, "assert pipe_rst err %d\n", err); > + return err; > + } > + > + pcie_write(port, (0xf << 20) | (0x1 << 16) | PCIE_CLIENT_GEN_SEL_2 | > + (0x1 << 19) | (0x1 << 3) | > + PCIE_CLIENT_MODE_RC | > + PCIE_CLIENT_CONF_LANE_NUM(port->lanes) | > + PCIE_CLIENT_CONF_ENABLE, PCIE_CLIENT_BASE); > + > + err = reset_control_deassert(port->phy_rst); > + if (err) { > + dev_err(port->dev, "deassert phy_rst err %d\n", err); > + return err; > + } > + regmap_write(port->grf, port->pcie_conf, > + (0x3f << 17) | (0x10 << 1)); > + err = -EINVAL; > + while (time_before(jiffies, timeout)) { > + regmap_read(port->grf, port->pcie_status, &status); > + if ((status & (1 << 9))) { > + dev_info(port->dev, "pll locked!\n"); > + err = 0; > + break; > + } > + } > + if (err) { > + dev_err(port->dev, "pll lock timeout!\n"); > + return err; > + } > + pcie_pb_wr_cfg(port, 0x10, 0x8); > + pcie_pb_wr_cfg(port, 0x12, 0x8); > + > + err = -ETIMEDOUT; > + while (time_before(jiffies, timeout)) { > + regmap_read(port->grf, port->pcie_status, &status); > + if (!(status & (1 << 10))) { > + dev_info(port->dev, "pll output enable done!\n"); > + err = 0; > + break; > + } > + } > + > + if (err) { > + dev_err(port->dev, "pll output enable timeout!\n"); > + return err; > + } > + > + regmap_write(port->grf, port->pcie_conf, > + (0x3f << 17) | (0x10 << 1)); > + err = -EINVAL; > + while (time_before(jiffies, timeout)) { > + regmap_read(port->grf, port->pcie_status, &status); > + if ((status & (1 << 9))) { > + dev_info(port->dev, "pll relocked!\n"); > + err = 0; > + break; > + } > + } > + if (err) { > + dev_err(port->dev, "pll relock timeout!\n"); > + return err; > + } There's a bunch of these "regmap_write, loop waiting for a bit, error check"(s) May be have an inline function for it? > + > + err = reset_control_deassert(port->core_rst); > + if (err) { > + dev_err(port->dev, "deassert core_rst err %d\n", err); > + return err; > + } > + err = reset_control_deassert(port->mgmt_rst); > + if (err) { > + dev_err(port->dev, "deassert mgmt_rst err %d\n", err); > + return err; > + } > + err = reset_control_deassert(port->mgmt_sticky_rst); > + if (err) { > + dev_err(port->dev, "deassert mgmt_sticky_rst err %d\n", err); > + return err; > + } > + err = reset_control_deassert(port->pipe_rst); > + if (err) { > + dev_err(port->dev, "deassert pipe_rst err %d\n", err); > + return err; > + } > + > + pcie_write(port, 1 << 17 | 1 << 1, PCIE_CLIENT_BASE); > + > + gpiod_set_value(port->ep_gpio, 1); > + err = -ETIMEDOUT; > + while (time_before(jiffies, timeout)) { > + status = pcie_read(port, PCIE_CLIENT_BASIC_STATUS1); > + if (((status >> 20) & 0x3) == 0x3) { > + dev_info(port->dev, "pcie link training gen1 pass!\n"); > + err = 0; > + break; > + } > + } > + if (err) { > + dev_err(port->dev, "pcie link training gen1 timeout!\n"); > + return err; > + } > + > + status = pcie_read(port, 0x9000d0); > + status |= 0x20; > + pcie_write(port, status, 0x9000d0); > + err = -ETIMEDOUT; > + while (time_before(jiffies, timeout)) { > + status = pcie_read(port, PCIE_CORE_CTRL_MGMT_BASE); > + if (((status >> 3) & 0x3) == 0x1) { > + dev_info(port->dev, "pcie link training gen2 pass!\n"); > + err = 0; > + break; > + } > + } > + if (err) > + dev_dbg(port->dev, "pcie link training gen2 timeout, force to gen1!\n"); > + > + if (((status >> 3) & 0x3) == 0x0) > + dev_info(port->dev, "pcie link 2.5!\n"); > + if (((status >> 3) & 0x3) == 0x1) > + dev_info(port->dev, "pcie link 5.0!\n"); > + > + status = pcie_read(port, PCIE_CORE_CTRL_MGMT_BASE); > + status = 0x1 << ((status >> 1) & 0x3); > + dev_info(port->dev, "current link width is x%d\n", status); > + > + status = pcie_pb_rd_cfg(port, 0x30); > + if (!((status >> 11) & 0x1)) > + dev_dbg(port->dev, "lane A is used\n"); > + else > + regmap_write(port->grf, port->pcie_laneoff, > + (0x1 << 19) | (0x1 << 3)); > + > + status = pcie_pb_rd_cfg(port, 0x31); > + if (!((status >> 11) & 0x1)) > + dev_dbg(port->dev, "lane B is used\n"); > + else > + regmap_write(port->grf, port->pcie_laneoff, > + (0x2 << 19) | (0x2 << 3)); > + > + status = pcie_pb_rd_cfg(port, 0x32); > + if (!((status >> 11) & 0x1)) > + dev_dbg(port->dev, "lane C is used\n"); > + else > + regmap_write(port->grf, port->pcie_laneoff, > + (0x4 << 19) | (0x4 << 3)); > + > + status = pcie_pb_rd_cfg(port, 0x33); > + if (!((status >> 11) & 0x1)) > + dev_dbg(port->dev, "lane D is used\n"); > + else > + regmap_write(port->grf, port->pcie_laneoff, > + (0x8 << 19) | (0x8 << 3)); Can we have a loop looping over all the lanes instead of this? Also, you seem to be checking and initializing all lanes instead of referring to port->lanes? > + return 0; > +} > + > +/** > + * rockchip_pcie_parse_dt - Parse Device tree > + * @port: PCIe port information > + * > + * Return: '0' on success and error value on failure > + */ > +static int rockchip_pcie_parse_dt(struct rockchip_pcie_port *port) > +{ > + struct device *dev = port->dev; > + struct device_node *node = dev->of_node; > + struct resource regs; > + unsigned int pcie_conf; > + unsigned int pcie_status; > + unsigned int pcie_laneoff; > + int err; > + > + err = of_address_to_resource(node, 0, ®s); > + if (err) { > + dev_err(dev, "missing \"reg\" property\n"); This error message is exactly the same as the one below. May be say reg0 or reg1? > + return err; > + } > + > + port->reg_base = devm_ioremap_resource(dev, ®s); > + if (IS_ERR(port->reg_base)) > + return PTR_ERR(port->reg_base); > + > + err = of_address_to_resource(node, 1, ®s); > + if (err) { > + dev_err(dev, "missing \"reg\" property\n"); > + return err; > + } > + > + port->apb_base = devm_ioremap_resource(dev, ®s); > + if (IS_ERR(port->apb_base)) > + return PTR_ERR(port->apb_base); > + > + port->grf = syscon_regmap_lookup_by_phandle(dev->of_node, > + "rockchip,grf"); > + if (IS_ERR(port->grf)) { > + dev_err(dev, "Missing rockchip,grf property\n"); > + return PTR_ERR(port->grf); > + } > + > + if (of_property_read_u32(node, "pcie-conf", &pcie_conf)) { > + dev_err(dev, "missing pcie-conf property in node %s\n", > + node->name); > + return -EINVAL; > + } > + > + port->pcie_conf = pcie_conf; > + > + if (of_property_read_u32(node, "pcie-status", &pcie_status)) { > + dev_err(dev, "missing pcie-status property in node %s\n", > + node->name); > + return -EINVAL; > + } > + > + port->pcie_status = pcie_status; > + > + if (of_property_read_u32(node, "pcie-laneoff", &pcie_laneoff)) { > + dev_err(dev, "missing pcie-laneoff property in node %s\n", > + node->name); > + return -EINVAL; > + } > + > + port->pcie_laneoff = pcie_laneoff; > + > + port->lanes = 1; > + err = of_property_read_u32(node, "num-lanes", &port->lanes); > + if (!err && ((port->lanes == 0) || > + (port->lanes == 3) || > + (port->lanes > 4))) { > + dev_info(dev, "invalid num-lanes, default use one lane\n"); > + port->lanes = 1; > + } > + > + port->phy_rst = devm_reset_control_get(dev, "phy-rst"); > + if (IS_ERR(port->phy_rst)) { > + if (PTR_ERR(port->phy_rst) != -EPROBE_DEFER) > + dev_err(dev, "missing phy-rst property in node %s\n", > + node->name); > + err = PTR_ERR(port->phy_rst); > + goto err_aclk_pcie; > + } > + > + port->core_rst = devm_reset_control_get(dev, "core-rst"); > + if (IS_ERR(port->core_rst)) { > + if (PTR_ERR(port->core_rst) != -EPROBE_DEFER) > + dev_err(dev, "missing core-rst property in node %s\n", > + node->name); > + err = PTR_ERR(port->core_rst); > + goto err_aclk_pcie; > + } > + > + port->mgmt_rst = devm_reset_control_get(dev, "mgmt-rst"); > + if (IS_ERR(port->mgmt_rst)) { > + if (PTR_ERR(port->mgmt_rst) != -EPROBE_DEFER) > + dev_err(dev, "missing mgmt-rst property in node %s\n", > + node->name); > + err = PTR_ERR(port->mgmt_rst); > + goto err_aclk_pcie; > + } > + > + port->mgmt_sticky_rst = devm_reset_control_get(dev, "mgmt-sticky-rst"); > + if (IS_ERR(port->mgmt_sticky_rst)) { > + if (PTR_ERR(port->mgmt_sticky_rst) != -EPROBE_DEFER) > + dev_err(dev, "missing mgmt-sticky-rst property in node %s\n", > + node->name); > + err = PTR_ERR(port->mgmt_sticky_rst); > + goto err_aclk_pcie; > + } > + > + port->pipe_rst = devm_reset_control_get(dev, "pipe-rst"); > + if (IS_ERR(port->pipe_rst)) { > + if (PTR_ERR(port->pipe_rst) != -EPROBE_DEFER) > + dev_err(dev, "missing pipe-rst property in node %s\n", > + node->name); > + err = PTR_ERR(port->pipe_rst); > + goto err_aclk_pcie; > + } > + > + port->ep_gpio = gpiod_get(dev, "ep", GPIOD_OUT_HIGH); > + if (IS_ERR(port->ep_gpio)) { > + dev_err(dev, "missing ep-gpios property in node %s\n", > + node->name); > + return PTR_ERR(port->ep_gpio); > + } > + > + port->aclk_pcie = devm_clk_get(dev, "aclk_pcie"); > + if (IS_ERR(port->aclk_pcie)) { > + dev_err(dev, "aclk_pcie clock not found.\n"); > + return PTR_ERR(port->aclk_pcie); > + } > + > + port->aclk_perf_pcie = devm_clk_get(dev, "aclk_perf_pcie"); > + if (IS_ERR(port->aclk_perf_pcie)) { > + dev_err(dev, "aclk_perf_pcie clock not found.\n"); > + return PTR_ERR(port->aclk_perf_pcie); > + } > + > + port->hclk_pcie = devm_clk_get(dev, "hclk_pcie"); > + if (IS_ERR(port->hclk_pcie)) { > + dev_err(dev, "hclk_pcie clock not found.\n"); > + return PTR_ERR(port->hclk_pcie); > + } > + > + port->clk_pciephy_ref = devm_clk_get(dev, "clk_pciephy_ref"); > + if (IS_ERR(port->clk_pciephy_ref)) { > + dev_err(dev, "clk_pciephy_ref clock not found.\n"); > + return PTR_ERR(port->clk_pciephy_ref); > + } > + > + err = clk_prepare_enable(port->aclk_pcie); > + if (err) { > + dev_err(dev, "Unable to enable aclk_pcie clock.\n"); > + goto err_aclk_pcie; > + } > + > + err = clk_prepare_enable(port->aclk_perf_pcie); > + if (err) { > + dev_err(dev, "Unable to enable aclk_perf_pcie clock.\n"); > + goto err_aclk_perf_pcie; > + } > + > + err = clk_prepare_enable(port->hclk_pcie); > + if (err) { > + dev_err(dev, "Unable to enable hclk_pcie clock.\n"); > + goto err_hclk_pcie; > + } > + > + err = clk_prepare_enable(port->clk_pciephy_ref); > + if (err) { > + dev_err(dev, "Unable to enable hclk_pcie clock.\n"); > + goto err_pciephy_ref; > + } IMHO, the code in rockchip_pcie_parse_dt() is doing much more than what the function name says i.e. "parsing the dt". May be move the clk_prepare_enable() calls out (and possibly others). > + > + return 0; > + > +err_pciephy_ref: > + clk_disable_unprepare(port->hclk_pcie); > +err_hclk_pcie: > + clk_disable_unprepare(port->aclk_perf_pcie); > +err_aclk_perf_pcie: > + clk_disable_unprepare(port->aclk_pcie); > +err_aclk_pcie: > + return err; > +} > + > +static void rockchip_pcie_msi_enable(struct rockchip_pcie_port *pp) > +{ This function seems to be (only) parsing the dt? If so we can move its code to parse_dt function? > + struct device_node *msi_node; > + > + msi_node = of_parse_phandle(pp->dev->of_node, > + "msi-parent", 0); > + if (!msi_node) > + return; > + > + pp->msi = of_pci_find_msi_chip_by_node(msi_node); > + of_node_put(msi_node); > + > + if (pp->msi) > + pp->msi->dev = pp->dev; > +} > + > +static void rockchip_pcie_enable_interrupts(struct rockchip_pcie_port *pp) > +{ > + pcie_write(pp, (CLIENT_INTERRUPTS << 16) & > + (~CLIENT_INTERRUPTS), PCIE_CLIENT_INT_MASK); > + pcie_write(pp, CORE_INTERRUPTS, PCIE_CORE_INT_MASK); > +} > + > +static int rockchip_pcie_intx_map(struct irq_domain *domain, unsigned int irq, > + irq_hw_number_t hwirq) > +{ > + irq_set_chip_and_handler(irq, &dummy_irq_chip, handle_simple_irq); > + irq_set_chip_data(irq, domain->host_data); > + > + return 0; > +} > + > +static const struct irq_domain_ops intx_domain_ops = { > + .map = rockchip_pcie_intx_map, > +}; > + > +static int rockchip_pcie_init_irq_domain(struct rockchip_pcie_port *pp) > +{ > + struct device *dev = pp->dev; > + struct device_node *node = dev->of_node; > + struct device_node *pcie_intc_node = of_get_next_child(node, NULL); > + > + if (!pcie_intc_node) { > + dev_err(dev, "No PCIe Intc node found\n"); > + return PTR_ERR(pcie_intc_node); > + } > + pp->irq_domain = irq_domain_add_linear(pcie_intc_node, 4, > + &intx_domain_ops, pp); > + if (!pp->irq_domain) { > + dev_err(dev, "Failed to get a INTx IRQ domain\n"); > + return PTR_ERR(pp->irq_domain); > + } > + > + return 0; > +} > + > +static irqreturn_t rockchip_pcie_subsys_irq_handler(int irq, void *arg) > +{ > + struct rockchip_pcie_port *pp = arg; > + u32 reg; > + u32 sub_reg; > + > + reg = pcie_read(pp, PCIE_CLIENT_INT_STATUS); > + if (reg & LOC_INT) { > + dev_dbg(pp->dev, "local interrupt recived\n"); > + sub_reg = pcie_read(pp, PCIE_CORE_INT_STATUS); > + if (sub_reg & PRFPE) > + dev_dbg(pp->dev, "Parity error detected while reading from the PNP Receive FIFO RAM\n"); > + > + if (sub_reg & CRFPE) > + dev_dbg(pp->dev, "Parity error detected while reading from the Completion Receive FIFO RAM\n"); > + > + if (sub_reg & RRPE) > + dev_dbg(pp->dev, "Parity error detected while reading from Replay Buffer RAM\n"); > + > + if (sub_reg & PRFO) > + dev_dbg(pp->dev, "Overflow occurred in the PNP Receive FIFO\n"); > + > + if (sub_reg & CRFO) > + dev_dbg(pp->dev, "Overflow occurred in the Completion Receive FIFO\n"); > + > + if (sub_reg & RT) > + dev_dbg(pp->dev, "Replay timer timed out\n"); > + > + if (sub_reg & RTR) > + dev_dbg(pp->dev, "Replay timer rolled over after 4 transmissions of the same TLP\n"); > + > + if (sub_reg & PE) > + dev_dbg(pp->dev, "Phy error detected on receive side\n"); > + > + if (sub_reg & MTR) > + dev_dbg(pp->dev, "Malformed TLP received from the link\n"); > + > + if (sub_reg & UCR) > + dev_dbg(pp->dev, "Malformed TLP received from the link\n"); > + > + if (sub_reg & FCE) > + dev_dbg(pp->dev, "An error was observed in the flow control advertisements from the other side\n"); > + > + if (sub_reg & CT) > + dev_dbg(pp->dev, "A request timed out waiting for completion\n"); > + > + if (sub_reg & UTC) > + dev_dbg(pp->dev, "Unmapped TC error\n"); > + > + if (sub_reg & MMVC) > + dev_dbg(pp->dev, "MSI mask register changes\n"); > + > + pcie_write(pp, sub_reg, PCIE_CORE_INT_STATUS); > + } > + > + pcie_write(pp, reg, PCIE_CLIENT_INT_STATUS); > + > + return IRQ_HANDLED; > +} > + > +static irqreturn_t rockchip_pcie_client_irq_handler(int irq, void *arg) > +{ > + struct rockchip_pcie_port *pp = arg; > + u32 reg; > + > + reg = pcie_read(pp, PCIE_CLIENT_INT_STATUS); > + if (reg & LEGACY_DONE) > + dev_dbg(pp->dev, "legacy done interrupt recived\n"); > + > + if (reg & MSG_DONE) > + dev_dbg(pp->dev, "message done interrupt recived\n"); > + > + if (reg & HOT_RESET) > + dev_dbg(pp->dev, "hot reset interrupt recived\n"); > + > + if (reg & DPA_INT) > + dev_dbg(pp->dev, "dpa interrupt recived\n"); > + > + if (reg & FATAL_ERR) > + dev_dbg(pp->dev, "fatal error interrupt recived\n"); > + > + if (reg & DPA_INT) > + dev_dbg(pp->dev, "no fatal error interrupt recived\n"); > + > + if (reg & CORR_ERR) > + dev_dbg(pp->dev, "correctable error interrupt recived\n"); > + > + pcie_write(pp, reg, PCIE_CLIENT_INT_STATUS); This is a curosity question. You have 3 interrupt handlers, and in all 3 of them you are checking the same register (PCIE_CLIENT_INT_STATUS) and unconditionally clearing all the interrupts in that register. So I think if pci-sys and legacy are raised at the same time, the 1st interrupt handler that gets called may clear all interrupts and the second may find none to service. This may be OK, I'm wondering if this is the intention? > + > + return IRQ_HANDLED; > +} > + > +static irqreturn_t rockchip_pcie_legacy_int_handler(int irq, void *arg) > +{ > + struct rockchip_pcie_port *pp = arg; > + u32 reg; > + > + reg = pcie_read(pp, PCIE_CLIENT_INT_STATUS); > + reg = (reg & ROCKCHIP_PCIE_RPIFR1_INTR_MASK) >> > + ROCKCHIP_PCIE_RPIFR1_INTR_SHIFT; > + generic_handle_irq(irq_find_mapping(pp->irq_domain, ffs(reg))); > + > + pcie_write(pp, reg, PCIE_CLIENT_INT_STATUS); > + return IRQ_HANDLED; > +} > + > +static int rockchip_pcie_prog_ob_atu(struct rockchip_pcie_port *pp, > + int region_no, > + int type, u8 num_pass_bits, > + u32 lower_addr, u32 upper_addr) > +{ > + u32 ob_addr_0 = 0; > + u32 ob_addr_1 = 0; > + u32 ob_desc_0 = 0; > + u32 ob_desc_1 = 0; > + void __iomem *aw_base; > + > + if (!pp) > + return -EINVAL; > + if (region_no >= MAX_AXI_WRAPPER_REGION_NUM) > + return -EINVAL; > + if ((num_pass_bits + 1) < 8) > + return -EINVAL; > + if (num_pass_bits > 63) > + return -EINVAL; > + if (region_no == 0) { > + if (AXI_REGION_0_SIZE < (2ULL << num_pass_bits)) > + return -EINVAL; > + } > + if (region_no != 0) { > + if (AXI_REGION_SIZE < (2ULL << num_pass_bits)) > + return -EINVAL; > + } > + aw_base = pp->apb_base + PCIE_CORE_AXI_CONF_BASE; > + aw_base += (region_no << OB_REG_SIZE_SHIFT); > + > + ob_addr_0 = (ob_addr_0 & > + ~0x0000003fU) | (num_pass_bits & > + 0x0000003fU); > + ob_addr_0 = (ob_addr_0 & > + ~0xffffff00U) | (lower_addr & 0xffffff00U); > + ob_addr_1 = upper_addr; > + ob_desc_0 = (1 << 23 | type); > + > + writel(ob_addr_0, aw_base); > + writel(ob_addr_1, aw_base + 0x4); > + writel(ob_desc_0, aw_base + 0x8); > + writel(ob_desc_1, aw_base + 0xc); > + > + return 0; > +} > + > +static int rockchip_pcie_prog_ib_atu(struct rockchip_pcie_port *pp, > + int region_no, > + u8 num_pass_bits, > + u32 lower_addr, > + u32 upper_addr) > +{ > + u32 ib_addr_0 = 0; > + u32 ib_addr_1 = 0; > + void __iomem *aw_base; > + > + if (!pp) > + return -EINVAL; > + if (region_no > MAX_AXI_IB_ROOTPORT_REGION_NUM) > + return -EINVAL; > + if ((num_pass_bits + 1) < MIN_AXI_ADDR_BITS_PASSED) > + return -EINVAL; > + if (num_pass_bits > 63) > + return -EINVAL; > + aw_base = pp->apb_base + PCIE_CORE_AXI_INBOUND_BASE; > + aw_base += (region_no << IB_ROOT_PORT_REG_SIZE_SHIFT); > + ib_addr_0 = (ib_addr_0 & > + ~0x0000003fU) | (num_pass_bits & > + 0x0000003fU); > + > + ib_addr_0 = (ib_addr_0 & ~0xffffff00U) | > + ((lower_addr << 8) & 0xffffff00U); > + ib_addr_1 = upper_addr; > + writel(ib_addr_0, aw_base); > + writel(ib_addr_1, aw_base + 0x4); > + > + return 0; > +} > + > +static int rockchip_pcie_probe(struct platform_device *pdev) > +{ > + struct rockchip_pcie_port *port; > + struct device *dev = &pdev->dev; > + struct pci_bus *bus, *child; > + struct resource_entry *win; > + int reg_no; > + int err = 0; > + int irq; > + LIST_HEAD(res); > + > + if (!dev->of_node) > + return -ENODEV; > + > + port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL); > + if (!port) > + return -ENOMEM; > + > + irq = platform_get_irq_byname(pdev, "pcie-sys"); > + if (irq < 0) { > + dev_err(dev, "missing pcie_sys IRQ resource\n"); > + return -EINVAL; > + } > + err = devm_request_irq(dev, irq, rockchip_pcie_subsys_irq_handler, > + IRQF_SHARED, "pcie-sys", port); I am a little worried because you are installing all the 3 interrupt handlers, passing "port *" as the argument - even though the struct port is not initialized yet. Specifically the pointers (port->apb_base) that irq handler would dereference. Also, the irq domain is not initialized. May be I'm just cynical, but if there is an interrupt pending, I'm wondering if your interrupt handler can get called even before you get a chance to initialize your struct rockchip_pcie_port *port or irq domain? If so, it may crash. So may be better to first parse dt and initialize your structures before registering the interrupt handler(s)? > + if (err) { > + dev_err(dev, "failed to request pcie subsystem irq\n"); > + return err; > + } > + > + port->irq = platform_get_irq_byname(pdev, "pcie-legacy"); > + if (port->irq < 0) { > + dev_err(dev, "missing pcie_legacy IRQ resource\n"); > + return -EINVAL; > + } > + err = devm_request_irq(dev, port->irq, > + rockchip_pcie_legacy_int_handler, > + IRQF_SHARED, > + "pcie-legacy", > + port); > + if (err) { > + dev_err(&pdev->dev, "failed to request pcie-legacy irq\n"); > + return err; > + } > + > + irq = platform_get_irq_byname(pdev, "pcie-client"); > + if (irq < 0) { > + dev_err(dev, "missing pcie-client IRQ resource\n"); > + return -EINVAL; > + } > + err = devm_request_irq(dev, irq, rockchip_pcie_client_irq_handler, > + IRQF_SHARED, "pcie-client", port); > + if (err) { > + dev_err(dev, "failed to request pcie client irq\n"); > + return err; > + } > + > + port->dev = dev; > + err = rockchip_pcie_parse_dt(port); > + if (err) { > + dev_err(dev, "Parsing DT failed\n"); > + return err; > + } > + > + err = rockchip_pcie_init_port(port); > + if (err) > + return err; What about clocks? Do you need to clk_disable_unprepare() them on this failure or any subsequent failure in probe? > + > + platform_set_drvdata(pdev, port); > + > + rockchip_pcie_enable_interrupts(port); > + if (!IS_ENABLED(CONFIG_PCI_MSI)) { > + err = rockchip_pcie_init_irq_domain(port); > + if (err < 0) > + return err; > + } > + > + err = of_pci_get_host_bridge_resources(dev->of_node, 0, 0xff, > + &res, &port->io_base); > + if (err) > + return err; > + /* Get the I/O and memory ranges from DT */ > + resource_list_for_each_entry(win, &res) { > + switch (resource_type(win->res)) { > + case IORESOURCE_IO: > + port->io = win->res; > + port->io->name = "I/O"; > + port->io_size = resource_size(port->io); > + port->io_bus_addr = port->io->start - win->offset; > + err = pci_remap_iospace(port->io, port->io_base); > + if (err) { > + dev_warn(port->dev, "error %d: failed to map resource %pR\n", > + err, port->io); > + continue; > + } > + break; > + case IORESOURCE_MEM: > + port->mem = win->res; > + port->mem->name = "MEM"; > + port->mem_size = resource_size(port->mem); > + port->mem_bus_addr = port->mem->start - win->offset; > + break; > + case 0: > + port->cfg = win->res; > + break; > + case IORESOURCE_BUS: > + port->busn = win->res; > + break; > + default: > + continue; > + } > + } > + > + pcie_write(port, 0x6040000, PCIE_RC_CONFIG_BASE + 0x8); > + pcie_write(port, 0x0, PCIE_CORE_CTRL_MGMT_BASE + 0x300); > + > + pcie_write(port, (RC_REGION_0_ADDR_TRANS_L + RC_REGION_0_PASS_BITS), > + PCIE_CORE_AXI_CONF_BASE); > + pcie_write(port, RC_REGION_0_ADDR_TRANS_H, > + PCIE_CORE_AXI_CONF_BASE + 0x4); > + pcie_write(port, 0x0080000a, PCIE_CORE_AXI_CONF_BASE + 0x8); > + pcie_write(port, 0x00000000, PCIE_CORE_AXI_CONF_BASE + 0xc); > + > + for (reg_no = 0; reg_no < (port->mem_size >> 20); reg_no++) { > + err = rockchip_pcie_prog_ob_atu(port, reg_no + 1, > + AXI_WRAPPER_MEM_WRITE, > + 20 - 1, > + port->mem_bus_addr + > + (reg_no << 20), > + 0); > + if (err) { > + dev_err(dev, "Program RC outbound atu failed\n"); > + return err; > + } > + } > + > + err = rockchip_pcie_prog_ib_atu(port, 2, 32 - 1, 0x0, 0); > + if (err) { > + dev_err(dev, "Program RC inbound atu failed\n"); > + return err; > + } > + > + rockchip_pcie_msi_enable(port); > + > + port->root_bus_nr = port->busn->start; > + if (IS_ENABLED(CONFIG_PCI_MSI)) { > + bus = pci_scan_root_bus_msi(port->dev, port->root_bus_nr, > + &rockchip_pcie_ops, port, &res, > + port->msi); > + } else { > + bus = pci_scan_root_bus(&pdev->dev, 0, > + &rockchip_pcie_ops, port, &res); > + } > + if (!bus) > + return -ENOMEM; > + > + if (!pci_has_flag(PCI_PROBE_ONLY)) { > + pci_bus_size_bridges(bus); > + pci_bus_assign_resources(bus); > + list_for_each_entry(child, &bus->children, node) > + pcie_bus_configure_settings(child); > + } > + > + pci_bus_add_devices(bus); > + > + return err; > +} > + > +static int rockchip_pcie_remove(struct platform_device *pdev) > +{ > + struct rockchip_pcie_port *port = platform_get_drvdata(pdev); > + > + clk_disable_unprepare(port->hclk_pcie); > + clk_disable_unprepare(port->aclk_perf_pcie); > + clk_disable_unprepare(port->aclk_pcie); > + clk_disable_unprepare(port->clk_pciephy_ref); > + > + return 0; > +} > + > +static const struct of_device_id rockchip_pcie_of_match[] = { > + { .compatible = "rockchip,rk3399-pcie", }, > + {} > +}; Please add a MODULE_DEVICE_TABLE() here. > + > +static struct platform_driver rockchip_pcie_driver = { > + .driver = { > + .name = "rockchip-pcie", > + .of_match_table = rockchip_pcie_of_match, > + .suppress_bind_attrs = true, > + }, > + .probe = rockchip_pcie_probe, > + .remove = rockchip_pcie_remove, > +}; > +module_platform_driver(rockchip_pcie_driver); > + > +MODULE_AUTHOR("Rockchip Inc"); > +MODULE_DESCRIPTION("Rockchip AXI PCIe driver"); > +MODULE_LICENSE("GPL v2"); -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On 2016/5/24 21:03, Arnd Bergmann wrote: > On Friday, May 20, 2016 6:29:16 PM CEST Shawn Lin wrote: > >> +static int rockchip_pcie_wr_own_conf(struct rockchip_pcie_port *pp, >> + int where, int size, u32 val) >> +{ >> + u32 tmp; >> + int offset; >> + >> + offset = (where & (~0x3)); >> + tmp = readl(pp->apb_base + PCIE_RC_CONFIG_BASE + offset); >> + if (size == 4) { >> + writel(val, pp->apb_base + PCIE_RC_CONFIG_BASE + where); >> + } else if (size == 2) { >> + if (where & 0x2) >> + tmp = ((tmp & 0xffff) | (val << 16)); >> + else >> + tmp = ((tmp & 0xffff0000) | val); >> + >> + writel(tmp, pp->apb_base + PCIE_RC_CONFIG_BASE + offset); >> + } else if (size == 1) { >> + if ((where & 0x3) == 0) >> + tmp = ((tmp & (~0xff)) | val); >> + else if ((where & 0x3) == 1) >> + tmp = ((tmp & (~0xff00)) | (val << 8)); >> + else if ((where & 0x3) == 2) >> + tmp = ((tmp & (~0xff0000)) | (val << 16)); >> + else if ((where & 0x3) == 3) >> + tmp = ((tmp & (~0xff000000)) | (val << 24)); >> + >> + writel(tmp, pp->apb_base + PCIE_RC_CONFIG_BASE + offset); >> + } else { >> + return PCIBIOS_BAD_REGISTER_NUMBER; >> + } >> + return PCIBIOS_SUCCESSFUL; >> +} > > Why can't you access the individual sub-word registers here? Our soc not support byte and word write in these region. so we have to use writel() to write these registers. > >> + >> +static int rockchip_pcie_rd_other_conf(struct rockchip_pcie_port *pp, >> + struct pci_bus *bus, u32 devfn, >> + int where, int size, u32 *val) >> +{ >> + u32 busdev; >> + >> + busdev = PCIE_ECAM_ADDR(bus->number, PCI_SLOT(devfn), >> + PCI_FUNC(devfn), where); >> + >> + if (busdev & (size - 1)) { >> + *val = 0; >> + return PCIBIOS_BAD_REGISTER_NUMBER; >> + } >> + >> + if (size == 4) { >> + *val = readl(pp->reg_base + busdev); >> + } else if (size == 2) { >> + *val = readw(pp->reg_base + busdev); >> + } else if (size == 1) { >> + *val = readb(pp->reg_base + busdev); >> + } else { >> + *val = 0; >> + return PCIBIOS_BAD_REGISTER_NUMBER; >> + } >> + return PCIBIOS_SUCCESSFUL; >> +} >> + > > This looks like the normal ECAM operations, you could just call those. I read ECAM reference code, I found it not support ioremap config space for each bus individually on 64-bit systems. Our soc is 64-bit system, and bus0 config space base address is 0xfda00000, bus1 base address is 0xf8100000. So I think it is not normal ECAM operations, I do not know if I have understood correctly? > >> + while (time_before(jiffies, timeout)) { >> + regmap_read(port->grf, port->pcie_status, &status); >> + if ((status & (1 << 9))) { >> + dev_info(port->dev, "pll locked!\n"); >> + err = 0; >> + break; >> + } >> + } > > Maybe add an msleep(1) here to avoid busy-looping? Yeah, its a good suggestion. > > >> + for (reg_no = 0; reg_no < (port->mem_size >> 20); reg_no++) { >> + err = rockchip_pcie_prog_ob_atu(port, reg_no + 1, >> + AXI_WRAPPER_MEM_WRITE, >> + 20 - 1, >> + port->mem_bus_addr + >> + (reg_no << 20), >> + 0); >> + if (err) { >> + dev_err(dev, "Program RC outbound atu failed\n"); >> + return err; >> + } >> + } > > What if there is more than one outbound memory window, e.g. prefetchable > and non-prefetchable? > > Where do you set the I/O window? Yeah, our soc support these outbound windows, we will support these windows next patch. > >> + err = rockchip_pcie_prog_ib_atu(port, 2, 32 - 1, 0x0, 0); >> + if (err) { >> + dev_err(dev, "Program RC inbound atu failed\n"); >> + return err; >> + } > > And this doesn't seem to reflect the DMA ranges. Our soc support all the ram region use as DMA zone. SO I set all the 32 bits bypass for the inbound ATS. > >> + >> + port->root_bus_nr = port->busn->start; >> + if (IS_ENABLED(CONFIG_PCI_MSI)) { >> + bus = pci_scan_root_bus_msi(port->dev, port->root_bus_nr, >> + &rockchip_pcie_ops, port, &res, >> + port->msi); >> + } else { >> + bus = pci_scan_root_bus(&pdev->dev, 0, >> + &rockchip_pcie_ops, port, &res); > > > PCI_MSI is selected unconditionally from Kconfig for this driver, so no > need for the compile-time check here. Yeah, we will delete the compile-time check in next patch. > >> + >> +static int rockchip_pcie_remove(struct platform_device *pdev) >> +{ >> + struct rockchip_pcie_port *port = platform_get_drvdata(pdev); >> + >> + clk_disable_unprepare(port->hclk_pcie); >> + clk_disable_unprepare(port->aclk_perf_pcie); >> + clk_disable_unprepare(port->aclk_pcie); >> + clk_disable_unprepare(port->clk_pciephy_ref); >> + >> + return 0; >> +} > > You don't seem to remove the child devices here. Have you tried unloading the module? > I don't seem the remove function in other pcie host driver. I think this function could be deleted. > Arnd > > > > > Best Regards Wenrui Li -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> > > >> + > >> +static int rockchip_pcie_rd_other_conf(struct rockchip_pcie_port *pp, > >> + struct pci_bus *bus, u32 devfn, > >> + int where, int size, u32 *val) > >> +{ > >> + u32 busdev; > >> + > >> + busdev = PCIE_ECAM_ADDR(bus->number, PCI_SLOT(devfn), > >> + PCI_FUNC(devfn), where); > >> + > >> + if (busdev & (size - 1)) { > >> + *val = 0; > >> + return PCIBIOS_BAD_REGISTER_NUMBER; > >> + } > >> + > >> + if (size == 4) { > >> + *val = readl(pp->reg_base + busdev); > >> + } else if (size == 2) { > >> + *val = readw(pp->reg_base + busdev); > >> + } else if (size == 1) { > >> + *val = readb(pp->reg_base + busdev); > >> + } else { > >> + *val = 0; > >> + return PCIBIOS_BAD_REGISTER_NUMBER; > >> + } > >> + return PCIBIOS_SUCCESSFUL; > >> +} > >> + > > > > This looks like the normal ECAM operations, you could just call those. > > I read ECAM reference code, I found it not support ioremap config space > for each bus individually on 64-bit systems. Our soc is 64-bit system, > and bus0 config space base address is 0xfda00000, bus1 base address is > 0xf8100000. So I think it is not normal ECAM operations, I do not know > if I have understood correctly? > Hi, I think Arnd was suggesting to use generic config read/write calls, pci_generic_config_read/pci_generic_config_write which does above functionality. Bharat -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On 2016/5/27 15:13, Bharat Kumar Gogada wrote: >>> >>>> + >>>> +static int rockchip_pcie_rd_other_conf(struct rockchip_pcie_port *pp, >>>> + struct pci_bus *bus, u32 devfn, >>>> + int where, int size, u32 *val) >>>> +{ >>>> + u32 busdev; >>>> + >>>> + busdev = PCIE_ECAM_ADDR(bus->number, PCI_SLOT(devfn), >>>> + PCI_FUNC(devfn), where); >>>> + >>>> + if (busdev & (size - 1)) { >>>> + *val = 0; >>>> + return PCIBIOS_BAD_REGISTER_NUMBER; >>>> + } >>>> + >>>> + if (size == 4) { >>>> + *val = readl(pp->reg_base + busdev); >>>> + } else if (size == 2) { >>>> + *val = readw(pp->reg_base + busdev); >>>> + } else if (size == 1) { >>>> + *val = readb(pp->reg_base + busdev); >>>> + } else { >>>> + *val = 0; >>>> + return PCIBIOS_BAD_REGISTER_NUMBER; >>>> + } >>>> + return PCIBIOS_SUCCESSFUL; >>>> +} >>>> + >>> >>> This looks like the normal ECAM operations, you could just call those. >> >> I read ECAM reference code, I found it not support ioremap config space >> for each bus individually on 64-bit systems. Our soc is 64-bit system, >> and bus0 config space base address is 0xfda00000, bus1 base address is >> 0xf8100000. So I think it is not normal ECAM operations, I do not know >> if I have understood correctly? >> > Hi, > > I think Arnd was suggesting to use generic config read/write calls, pci_generic_config_read/pci_generic_config_write > which does above functionality. Yeah, I seem the pci_generic_config_write use writew/writeb for byte and word write. but our SOC not support byte and word write in RC config spcace. So I redefine the the pci_ops.write > > Bharat > > > -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[+Lorenzo] On 20/05/16 11:29, Shawn Lin wrote: > RK3399 has a PCIe controller which can be used as Root Complex. > This driver supports a PCIe controller as Root Complex mode. > > Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com> > --- > > drivers/pci/host/Kconfig | 12 + > drivers/pci/host/Makefile | 1 + > drivers/pci/host/pcie-rockchip.c | 1181 ++++++++++++++++++++++++++++++++++++++ > 3 files changed, 1194 insertions(+) > create mode 100644 drivers/pci/host/pcie-rockchip.c > > diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig > index 8e4f038..76447a8 100644 > --- a/drivers/pci/host/Kconfig > +++ b/drivers/pci/host/Kconfig > @@ -245,4 +245,16 @@ config PCIE_ARMADA_8K > Designware hardware and therefore the driver re-uses the > Designware core functions to implement the driver. > > +config PCIE_ROCKCHIP > + bool "Rockchip PCIe controller" > + depends on ARM64 && ARCH_ROCKCHIP > + depends on OF > + select MFD_SYSCON > + select PCI_MSI > + select PCI_MSI_IRQ_DOMAIN > + help > + Say Y here if you want internal PCI support on Rockchip SoC. > + There are 1 internal PCIe port available to support GEN2 with > + 4 slots. > + > endmenu > diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile > index a6f85e3..fb3871e 100644 > --- a/drivers/pci/host/Makefile > +++ b/drivers/pci/host/Makefile > @@ -29,3 +29,4 @@ obj-$(CONFIG_PCIE_QCOM) += pcie-qcom.o > obj-$(CONFIG_PCI_HOST_THUNDER_ECAM) += pci-thunder-ecam.o > obj-$(CONFIG_PCI_HOST_THUNDER_PEM) += pci-thunder-pem.o > obj-$(CONFIG_PCIE_ARMADA_8K) += pcie-armada8k.o > +obj-$(CONFIG_PCIE_ROCKCHIP) += pcie-rockchip.o > diff --git a/drivers/pci/host/pcie-rockchip.c b/drivers/pci/host/pcie-rockchip.c > new file mode 100644 > index 0000000..4063fd3 > --- /dev/null > +++ b/drivers/pci/host/pcie-rockchip.c > @@ -0,0 +1,1181 @@ > +/* > + * Rockchip AXI PCIe host controller driver > + * > + * Copyright (c) 2016 Rockchip, Inc. > + * > + * Author: Shawn Lin <shawn.lin@rock-chips.com> > + * Wenrui Li <wenrui.li@rock-chips.com> > + * Bits taken from Synopsys Designware Host controller driver and > + * ARM PCI Host generic driver. > + * > + * This program is free software: you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation, either version 2 of the License, or > + * (at your option) any later version. > + */ > +#include <linux/clk.h> > +#include <linux/delay.h> > +#include <linux/gpio/consumer.h> > +#include <linux/interrupt.h> > +#include <linux/irq.h> > +#include <linux/irqdomain.h> > +#include <linux/kernel.h> > +#include <linux/mfd/syscon.h> > +#include <linux/module.h> > +#include <linux/msi.h> > +#include <linux/of_address.h> > +#include <linux/of_device.h> > +#include <linux/of_pci.h> > +#include <linux/of_platform.h> > +#include <linux/of_irq.h> > +#include <linux/pci.h> > +#include <linux/platform_device.h> > +#include <linux/reset.h> > +#include <linux/regmap.h> > + > +#define REF_CLK_100MHZ (100 * 1000 * 1000) > +#define PCIE_CLIENT_BASE 0x0 > +#define PCIE_RC_CONFIG_BASE 0xa00000 > +#define PCIE_CORE_CTRL_MGMT_BASE 0x900000 > +#define PCIE_CORE_AXI_CONF_BASE 0xc00000 > +#define PCIE_CORE_AXI_INBOUND_BASE 0xc00800 > + > +#define PCIE_CLIENT_BASIC_STATUS0 0x44 > +#define PCIE_CLIENT_BASIC_STATUS1 0x48 > +#define PCIE_CLIENT_INT_MASK 0x4c > +#define PCIE_CLIENT_INT_STATUS 0x50 > +#define PCIE_CORE_INT_MASK 0x900210 > +#define PCIE_CORE_INT_STATUS 0x90020c > + > +/** Size of one AXI Region (not Region 0) */ > +#define AXI_REGION_SIZE (0x1 << 20) > +/** Overall size of AXI area */ > +#define AXI_OVERALL_SIZE (64 * (0x1 << 20)) > +/** Size of Region 0, equal to sum of sizes of other regions */ > +#define AXI_REGION_0_SIZE (32 * (0x1 << 20)) > +#define OB_REG_SIZE_SHIFT 5 > +#define IB_ROOT_PORT_REG_SIZE_SHIFT 3 > + > +#define AXI_WRAPPER_IO_WRITE 0x6 > +#define AXI_WRAPPER_MEM_WRITE 0x2 > +#define MAX_AXI_IB_ROOTPORT_REGION_NUM 3 > +#define MIN_AXI_ADDR_BITS_PASSED 8 > + > +#define ROCKCHIP_PCIE_RPIFR1_INTR_MASK GENMASK(8, 5) > +#define ROCKCHIP_PCIE_RPIFR1_INTR_SHIFT 5 > +#define CLIENT_INTERRUPTS \ > + (LOC_INT | INTA | INTB | INTC | INTD |\ > + CORR_ERR | NFATAL_ERR | FATAL_ERR | DPA_INT | \ > + HOT_RESET | MSG_DONE | LEGACY_DONE) > +#define CORE_INTERRUPTS \ > + (PRFPE | CRFPE | RRPE | CRFO | RT | RTR | \ > + PE | MTR | UCR | FCE | CT | UTC | MMVC) > +#define PWR_STCG BIT(0) > +#define HOT_PLUG BIT(1) > +#define PHY_INT BIT(2) > +#define UDMA_INT BIT(3) > +#define LOC_INT BIT(4) > +#define INTA BIT(5) > +#define INTB BIT(6) > +#define INTC BIT(7) > +#define INTD BIT(8) > +#define CORR_ERR BIT(9) > +#define NFATAL_ERR BIT(10) > +#define FATAL_ERR BIT(11) > +#define DPA_INT BIT(12) > +#define HOT_RESET BIT(13) > +#define MSG_DONE BIT(14) > +#define LEGACY_DONE BIT(15) > +#define PRFPE BIT(0) > +#define CRFPE BIT(1) > +#define RRPE BIT(2) > +#define PRFO BIT(3) > +#define CRFO BIT(4) > +#define RT BIT(5) > +#define RTR BIT(6) > +#define PE BIT(7) > +#define MTR BIT(8) > +#define UCR BIT(9) > +#define FCE BIT(10) > +#define CT BIT(11) > +#define UTC BIT(18) > +#define MMVC BIT(19) > + > +#define PCIE_ECAM_BUS(x) (((x) & 0xFF) << 20) > +#define PCIE_ECAM_DEV(x) (((x) & 0x1F) << 15) > +#define PCIE_ECAM_FUNC(x) (((x) & 0x7) << 12) > +#define PCIE_ECAM_REG(x) (((x) & 0xFFF) << 0) > +#define PCIE_ECAM_ADDR(bus, dev, func, reg) \ > + (PCIE_ECAM_BUS(bus) | PCIE_ECAM_DEV(dev) | \ > + PCIE_ECAM_FUNC(func) | PCIE_ECAM_REG(reg)) > + > +#define RC_REGION_0_ADDR_TRANS_H 0x00000000 > +#define RC_REGION_0_ADDR_TRANS_L 0x00000000 > +#define RC_REGION_0_PASS_BITS (25 - 1) > +#define RC_REGION_1_ADDR_TRANS_H 0x00000000 > +#define RC_REGION_1_ADDR_TRANS_L 0x00400000 > +#define RC_REGION_1_PASS_BITS (20 - 1) > +#define MAX_AXI_WRAPPER_REGION_NUM 33 > +#define PCIE_CLIENT_CONF_ENABLE BIT(0) > +#define PCIE_CLIENT_CONF_LANE_NUM(x) ((x / 2) << 4) > +#define PCIE_CLIENT_MODE_RC BIT(6) > +#define PCIE_CLIENT_GEN_SEL_2 BIT(7) > +#define PCIE_CLIENT_GEN_SEL_1 0x0 > + > +struct rockchip_pcie_port { > + void __iomem *reg_base; > + void __iomem *apb_base; > + struct regmap *grf; > + unsigned int pcie_conf; > + unsigned int pcie_status; > + unsigned int pcie_laneoff; > + struct reset_control *phy_rst; > + struct reset_control *core_rst; > + struct reset_control *mgmt_rst; > + struct reset_control *mgmt_sticky_rst; > + struct reset_control *pipe_rst; > + struct clk *aclk_pcie; > + struct clk *aclk_perf_pcie; > + struct clk *hclk_pcie; > + struct clk *clk_pciephy_ref; > + struct gpio_desc *ep_gpio; > + u32 lanes; > + resource_size_t io_base; > + struct resource *cfg; > + struct resource *io; > + struct resource *mem; > + struct resource *busn; > + phys_addr_t io_bus_addr; > + u32 io_size; > + phys_addr_t mem_bus_addr; > + u32 mem_size; > + u8 root_bus_nr; > + int irq; > + struct msi_controller *msi; Don't. struct msi_controller shouldn't be used in new code, and certainly not for an arm64 system. But even worse: your SoC seems to use a GICv3 ITS. Great. Which means you do not need any of that at all. > + > + struct device *dev; > + struct irq_domain *irq_domain; > +}; [...] > +static void rockchip_pcie_msi_enable(struct rockchip_pcie_port *pp) > +{ > + struct device_node *msi_node; > + > + msi_node = of_parse_phandle(pp->dev->of_node, > + "msi-parent", 0); > + if (!msi_node) > + return; > + > + pp->msi = of_pci_find_msi_chip_by_node(msi_node); How funny. The ITS is never registered there (actually, nobody seems to register anything there anymore), so this will always return NULL. This whole function is thus completely useless. > + of_node_put(msi_node); > + > + if (pp->msi) > + pp->msi->dev = pp->dev; > +} > + > +static void rockchip_pcie_enable_interrupts(struct rockchip_pcie_port *pp) > +{ > + pcie_write(pp, (CLIENT_INTERRUPTS << 16) & > + (~CLIENT_INTERRUPTS), PCIE_CLIENT_INT_MASK); > + pcie_write(pp, CORE_INTERRUPTS, PCIE_CORE_INT_MASK); > +} > + > +static int rockchip_pcie_intx_map(struct irq_domain *domain, unsigned int irq, > + irq_hw_number_t hwirq) > +{ > + irq_set_chip_and_handler(irq, &dummy_irq_chip, handle_simple_irq); > + irq_set_chip_data(irq, domain->host_data); > + > + return 0; > +} > + > +static const struct irq_domain_ops intx_domain_ops = { > + .map = rockchip_pcie_intx_map, > +}; > + > +static int rockchip_pcie_init_irq_domain(struct rockchip_pcie_port *pp) > +{ > + struct device *dev = pp->dev; > + struct device_node *node = dev->of_node; > + struct device_node *pcie_intc_node = of_get_next_child(node, NULL); That's really ugly, as it depends on the layout of your DT. > + > + if (!pcie_intc_node) { > + dev_err(dev, "No PCIe Intc node found\n"); > + return PTR_ERR(pcie_intc_node); > + } > + pp->irq_domain = irq_domain_add_linear(pcie_intc_node, 4, > + &intx_domain_ops, pp); Why can't you just register your host controller as the interrupt controller? You don't need an intermediate node for that. > + if (!pp->irq_domain) { > + dev_err(dev, "Failed to get a INTx IRQ domain\n"); > + return PTR_ERR(pp->irq_domain); > + } > + > + return 0; > +} > + > +static irqreturn_t rockchip_pcie_subsys_irq_handler(int irq, void *arg) > +{ > + struct rockchip_pcie_port *pp = arg; > + u32 reg; > + u32 sub_reg; > + > + reg = pcie_read(pp, PCIE_CLIENT_INT_STATUS); > + if (reg & LOC_INT) { > + dev_dbg(pp->dev, "local interrupt recived\n"); > + sub_reg = pcie_read(pp, PCIE_CORE_INT_STATUS); > + if (sub_reg & PRFPE) > + dev_dbg(pp->dev, "Parity error detected while reading from the PNP Receive FIFO RAM\n"); > + > + if (sub_reg & CRFPE) > + dev_dbg(pp->dev, "Parity error detected while reading from the Completion Receive FIFO RAM\n"); > + > + if (sub_reg & RRPE) > + dev_dbg(pp->dev, "Parity error detected while reading from Replay Buffer RAM\n"); > + > + if (sub_reg & PRFO) > + dev_dbg(pp->dev, "Overflow occurred in the PNP Receive FIFO\n"); > + > + if (sub_reg & CRFO) > + dev_dbg(pp->dev, "Overflow occurred in the Completion Receive FIFO\n"); > + > + if (sub_reg & RT) > + dev_dbg(pp->dev, "Replay timer timed out\n"); > + > + if (sub_reg & RTR) > + dev_dbg(pp->dev, "Replay timer rolled over after 4 transmissions of the same TLP\n"); > + > + if (sub_reg & PE) > + dev_dbg(pp->dev, "Phy error detected on receive side\n"); > + > + if (sub_reg & MTR) > + dev_dbg(pp->dev, "Malformed TLP received from the link\n"); > + > + if (sub_reg & UCR) > + dev_dbg(pp->dev, "Malformed TLP received from the link\n"); > + > + if (sub_reg & FCE) > + dev_dbg(pp->dev, "An error was observed in the flow control advertisements from the other side\n"); > + > + if (sub_reg & CT) > + dev_dbg(pp->dev, "A request timed out waiting for completion\n"); > + > + if (sub_reg & UTC) > + dev_dbg(pp->dev, "Unmapped TC error\n"); > + > + if (sub_reg & MMVC) > + dev_dbg(pp->dev, "MSI mask register changes\n"); > + > + pcie_write(pp, sub_reg, PCIE_CORE_INT_STATUS); > + } > + > + pcie_write(pp, reg, PCIE_CLIENT_INT_STATUS); > + > + return IRQ_HANDLED; > +} > + > +static irqreturn_t rockchip_pcie_client_irq_handler(int irq, void *arg) > +{ > + struct rockchip_pcie_port *pp = arg; > + u32 reg; > + > + reg = pcie_read(pp, PCIE_CLIENT_INT_STATUS); > + if (reg & LEGACY_DONE) > + dev_dbg(pp->dev, "legacy done interrupt recived\n"); > + > + if (reg & MSG_DONE) > + dev_dbg(pp->dev, "message done interrupt recived\n"); > + > + if (reg & HOT_RESET) > + dev_dbg(pp->dev, "hot reset interrupt recived\n"); > + > + if (reg & DPA_INT) > + dev_dbg(pp->dev, "dpa interrupt recived\n"); > + > + if (reg & FATAL_ERR) > + dev_dbg(pp->dev, "fatal error interrupt recived\n"); > + > + if (reg & DPA_INT) > + dev_dbg(pp->dev, "no fatal error interrupt recived\n"); > + > + if (reg & CORR_ERR) > + dev_dbg(pp->dev, "correctable error interrupt recived\n"); > + > + pcie_write(pp, reg, PCIE_CLIENT_INT_STATUS); > + > + return IRQ_HANDLED; > +} > + > +static irqreturn_t rockchip_pcie_legacy_int_handler(int irq, void *arg) > +{ > + struct rockchip_pcie_port *pp = arg; > + u32 reg; > + > + reg = pcie_read(pp, PCIE_CLIENT_INT_STATUS); > + reg = (reg & ROCKCHIP_PCIE_RPIFR1_INTR_MASK) >> > + ROCKCHIP_PCIE_RPIFR1_INTR_SHIFT; > + generic_handle_irq(irq_find_mapping(pp->irq_domain, ffs(reg))); NAK. What you have here is a chained interrupt controller, please implement it as such. > + > + pcie_write(pp, reg, PCIE_CLIENT_INT_STATUS); > + return IRQ_HANDLED; > +} > + > +static int rockchip_pcie_prog_ob_atu(struct rockchip_pcie_port *pp, > + int region_no, > + int type, u8 num_pass_bits, > + u32 lower_addr, u32 upper_addr) > +{ > + u32 ob_addr_0 = 0; > + u32 ob_addr_1 = 0; > + u32 ob_desc_0 = 0; > + u32 ob_desc_1 = 0; > + void __iomem *aw_base; > + > + if (!pp) > + return -EINVAL; > + if (region_no >= MAX_AXI_WRAPPER_REGION_NUM) > + return -EINVAL; > + if ((num_pass_bits + 1) < 8) > + return -EINVAL; > + if (num_pass_bits > 63) > + return -EINVAL; > + if (region_no == 0) { > + if (AXI_REGION_0_SIZE < (2ULL << num_pass_bits)) > + return -EINVAL; > + } > + if (region_no != 0) { > + if (AXI_REGION_SIZE < (2ULL << num_pass_bits)) > + return -EINVAL; > + } > + aw_base = pp->apb_base + PCIE_CORE_AXI_CONF_BASE; > + aw_base += (region_no << OB_REG_SIZE_SHIFT); > + > + ob_addr_0 = (ob_addr_0 & > + ~0x0000003fU) | (num_pass_bits & > + 0x0000003fU); > + ob_addr_0 = (ob_addr_0 & > + ~0xffffff00U) | (lower_addr & 0xffffff00U); > + ob_addr_1 = upper_addr; > + ob_desc_0 = (1 << 23 | type); > + > + writel(ob_addr_0, aw_base); > + writel(ob_addr_1, aw_base + 0x4); > + writel(ob_desc_0, aw_base + 0x8); > + writel(ob_desc_1, aw_base + 0xc); > + > + return 0; > +} > + > +static int rockchip_pcie_prog_ib_atu(struct rockchip_pcie_port *pp, > + int region_no, > + u8 num_pass_bits, > + u32 lower_addr, > + u32 upper_addr) > +{ > + u32 ib_addr_0 = 0; > + u32 ib_addr_1 = 0; > + void __iomem *aw_base; > + > + if (!pp) > + return -EINVAL; > + if (region_no > MAX_AXI_IB_ROOTPORT_REGION_NUM) > + return -EINVAL; > + if ((num_pass_bits + 1) < MIN_AXI_ADDR_BITS_PASSED) > + return -EINVAL; > + if (num_pass_bits > 63) > + return -EINVAL; > + aw_base = pp->apb_base + PCIE_CORE_AXI_INBOUND_BASE; > + aw_base += (region_no << IB_ROOT_PORT_REG_SIZE_SHIFT); > + ib_addr_0 = (ib_addr_0 & > + ~0x0000003fU) | (num_pass_bits & > + 0x0000003fU); > + > + ib_addr_0 = (ib_addr_0 & ~0xffffff00U) | > + ((lower_addr << 8) & 0xffffff00U); > + ib_addr_1 = upper_addr; > + writel(ib_addr_0, aw_base); > + writel(ib_addr_1, aw_base + 0x4); > + > + return 0; > +} > + > +static int rockchip_pcie_probe(struct platform_device *pdev) > +{ > + struct rockchip_pcie_port *port; > + struct device *dev = &pdev->dev; > + struct pci_bus *bus, *child; > + struct resource_entry *win; > + int reg_no; > + int err = 0; > + int irq; > + LIST_HEAD(res); > + > + if (!dev->of_node) > + return -ENODEV; > + > + port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL); > + if (!port) > + return -ENOMEM; > + > + irq = platform_get_irq_byname(pdev, "pcie-sys"); > + if (irq < 0) { > + dev_err(dev, "missing pcie_sys IRQ resource\n"); > + return -EINVAL; > + } > + err = devm_request_irq(dev, irq, rockchip_pcie_subsys_irq_handler, > + IRQF_SHARED, "pcie-sys", port); > + if (err) { > + dev_err(dev, "failed to request pcie subsystem irq\n"); > + return err; > + } > + > + port->irq = platform_get_irq_byname(pdev, "pcie-legacy"); > + if (port->irq < 0) { > + dev_err(dev, "missing pcie_legacy IRQ resource\n"); > + return -EINVAL; > + } > + err = devm_request_irq(dev, port->irq, > + rockchip_pcie_legacy_int_handler, > + IRQF_SHARED, > + "pcie-legacy", > + port); > + if (err) { > + dev_err(&pdev->dev, "failed to request pcie-legacy irq\n"); > + return err; > + } > + > + irq = platform_get_irq_byname(pdev, "pcie-client"); > + if (irq < 0) { > + dev_err(dev, "missing pcie-client IRQ resource\n"); > + return -EINVAL; > + } > + err = devm_request_irq(dev, irq, rockchip_pcie_client_irq_handler, > + IRQF_SHARED, "pcie-client", port); > + if (err) { > + dev_err(dev, "failed to request pcie client irq\n"); > + return err; > + } > + > + port->dev = dev; > + err = rockchip_pcie_parse_dt(port); > + if (err) { > + dev_err(dev, "Parsing DT failed\n"); > + return err; > + } > + > + err = rockchip_pcie_init_port(port); > + if (err) > + return err; > + > + platform_set_drvdata(pdev, port); > + > + rockchip_pcie_enable_interrupts(port); > + if (!IS_ENABLED(CONFIG_PCI_MSI)) { > + err = rockchip_pcie_init_irq_domain(port); > + if (err < 0) > + return err; > + } > + > + err = of_pci_get_host_bridge_resources(dev->of_node, 0, 0xff, > + &res, &port->io_base); > + if (err) > + return err; > + /* Get the I/O and memory ranges from DT */ > + resource_list_for_each_entry(win, &res) { > + switch (resource_type(win->res)) { > + case IORESOURCE_IO: > + port->io = win->res; > + port->io->name = "I/O"; > + port->io_size = resource_size(port->io); > + port->io_bus_addr = port->io->start - win->offset; > + err = pci_remap_iospace(port->io, port->io_base); > + if (err) { > + dev_warn(port->dev, "error %d: failed to map resource %pR\n", > + err, port->io); > + continue; > + } > + break; > + case IORESOURCE_MEM: > + port->mem = win->res; > + port->mem->name = "MEM"; > + port->mem_size = resource_size(port->mem); > + port->mem_bus_addr = port->mem->start - win->offset; > + break; > + case 0: > + port->cfg = win->res; > + break; > + case IORESOURCE_BUS: > + port->busn = win->res; > + break; > + default: > + continue; > + } > + } > + > + pcie_write(port, 0x6040000, PCIE_RC_CONFIG_BASE + 0x8); > + pcie_write(port, 0x0, PCIE_CORE_CTRL_MGMT_BASE + 0x300); > + > + pcie_write(port, (RC_REGION_0_ADDR_TRANS_L + RC_REGION_0_PASS_BITS), > + PCIE_CORE_AXI_CONF_BASE); > + pcie_write(port, RC_REGION_0_ADDR_TRANS_H, > + PCIE_CORE_AXI_CONF_BASE + 0x4); > + pcie_write(port, 0x0080000a, PCIE_CORE_AXI_CONF_BASE + 0x8); > + pcie_write(port, 0x00000000, PCIE_CORE_AXI_CONF_BASE + 0xc); > + > + for (reg_no = 0; reg_no < (port->mem_size >> 20); reg_no++) { > + err = rockchip_pcie_prog_ob_atu(port, reg_no + 1, > + AXI_WRAPPER_MEM_WRITE, > + 20 - 1, > + port->mem_bus_addr + > + (reg_no << 20), > + 0); > + if (err) { > + dev_err(dev, "Program RC outbound atu failed\n"); > + return err; > + } > + } > + > + err = rockchip_pcie_prog_ib_atu(port, 2, 32 - 1, 0x0, 0); > + if (err) { > + dev_err(dev, "Program RC inbound atu failed\n"); > + return err; > + } > + > + rockchip_pcie_msi_enable(port); > + > + port->root_bus_nr = port->busn->start; > + if (IS_ENABLED(CONFIG_PCI_MSI)) { > + bus = pci_scan_root_bus_msi(port->dev, port->root_bus_nr, > + &rockchip_pcie_ops, port, &res, > + port->msi); You don't need this. The infrastructure will do the right thing for you. > + } else { > + bus = pci_scan_root_bus(&pdev->dev, 0, > + &rockchip_pcie_ops, port, &res); > + } > + if (!bus) > + return -ENOMEM; > + > + if (!pci_has_flag(PCI_PROBE_ONLY)) { Why do you have catter for the PCI_PROBE_ONLY case? Nobody should ever use that for properly implemented HW. > + pci_bus_size_bridges(bus); > + pci_bus_assign_resources(bus); > + list_for_each_entry(child, &bus->children, node) > + pcie_bus_configure_settings(child); > + } > + > + pci_bus_add_devices(bus); > + > + return err; > +} > + > +static int rockchip_pcie_remove(struct platform_device *pdev) > +{ > + struct rockchip_pcie_port *port = platform_get_drvdata(pdev); > + > + clk_disable_unprepare(port->hclk_pcie); > + clk_disable_unprepare(port->aclk_perf_pcie); > + clk_disable_unprepare(port->aclk_pcie); > + clk_disable_unprepare(port->clk_pciephy_ref); > + > + return 0; > +} > + > +static const struct of_device_id rockchip_pcie_of_match[] = { > + { .compatible = "rockchip,rk3399-pcie", }, > + {} > +}; > + > +static struct platform_driver rockchip_pcie_driver = { > + .driver = { > + .name = "rockchip-pcie", > + .of_match_table = rockchip_pcie_of_match, > + .suppress_bind_attrs = true, > + }, > + .probe = rockchip_pcie_probe, > + .remove = rockchip_pcie_remove, > +}; > +module_platform_driver(rockchip_pcie_driver); > + > +MODULE_AUTHOR("Rockchip Inc"); > +MODULE_DESCRIPTION("Rockchip AXI PCIe driver"); > +MODULE_LICENSE("GPL v2"); > This definitely requires some rework for both the interrupt and MSI parts. I'll leave Lorenzo to comment on the PCI side of things. Thanks, M.
Hi: On 2016/5/27 20:25, Marc Zyngier Wrote: > [+Lorenzo] > > On 20/05/16 11:29, Shawn Lin wrote: >> RK3399 has a PCIe controller which can be used as Root Complex. >> This driver supports a PCIe controller as Root Complex mode. >> [....] >> +static int rockchip_pcie_init_irq_domain(struct rockchip_pcie_port *pp) >> +{ >> + struct device *dev = pp->dev; >> + struct device_node *node = dev->of_node; >> + struct device_node *pcie_intc_node = of_get_next_child(node, NULL); > > That's really ugly, as it depends on the layout of your DT. > >> + >> + if (!pcie_intc_node) { >> + dev_err(dev, "No PCIe Intc node found\n"); >> + return PTR_ERR(pcie_intc_node); >> + } >> + pp->irq_domain = irq_domain_add_linear(pcie_intc_node, 4, >> + &intx_domain_ops, pp); > > Why can't you just register your host controller as the interrupt > controller? You don't need an intermediate node for that. OK, the child node is really no need here, we will use the host controller as interrupt controller next patch. Thanks! > >> + if (!pp->irq_domain) { >> + dev_err(dev, "Failed to get a INTx IRQ domain\n"); >> + return PTR_ERR(pp->irq_domain); >> + } >> + >> + return 0; >> +} >> + >> +static irqreturn_t rockchip_pcie_subsys_irq_handler(int irq, void *arg) >> +{ >> + struct rockchip_pcie_port *pp = arg; >> + u32 reg; >> + u32 sub_reg; >> + >> + reg = pcie_read(pp, PCIE_CLIENT_INT_STATUS); >> + if (reg & LOC_INT) { >> + dev_dbg(pp->dev, "local interrupt recived\n"); >> + sub_reg = pcie_read(pp, PCIE_CORE_INT_STATUS); >> + if (sub_reg & PRFPE) >> + dev_dbg(pp->dev, "Parity error detected while reading from the PNP Receive FIFO RAM\n"); >> + >> + if (sub_reg & CRFPE) >> + dev_dbg(pp->dev, "Parity error detected while reading from the Completion Receive FIFO RAM\n"); >> + >> + if (sub_reg & RRPE) >> + dev_dbg(pp->dev, "Parity error detected while reading from Replay Buffer RAM\n"); >> + >> + if (sub_reg & PRFO) >> + dev_dbg(pp->dev, "Overflow occurred in the PNP Receive FIFO\n"); >> + >> + if (sub_reg & CRFO) >> + dev_dbg(pp->dev, "Overflow occurred in the Completion Receive FIFO\n"); >> + >> + if (sub_reg & RT) >> + dev_dbg(pp->dev, "Replay timer timed out\n"); >> + >> + if (sub_reg & RTR) >> + dev_dbg(pp->dev, "Replay timer rolled over after 4 transmissions of the same TLP\n"); >> + >> + if (sub_reg & PE) >> + dev_dbg(pp->dev, "Phy error detected on receive side\n"); >> + >> + if (sub_reg & MTR) >> + dev_dbg(pp->dev, "Malformed TLP received from the link\n"); >> + >> + if (sub_reg & UCR) >> + dev_dbg(pp->dev, "Malformed TLP received from the link\n"); >> + >> + if (sub_reg & FCE) >> + dev_dbg(pp->dev, "An error was observed in the flow control advertisements from the other side\n"); >> + >> + if (sub_reg & CT) >> + dev_dbg(pp->dev, "A request timed out waiting for completion\n"); >> + >> + if (sub_reg & UTC) >> + dev_dbg(pp->dev, "Unmapped TC error\n"); >> + >> + if (sub_reg & MMVC) >> + dev_dbg(pp->dev, "MSI mask register changes\n"); >> + >> + pcie_write(pp, sub_reg, PCIE_CORE_INT_STATUS); >> + } >> + >> + pcie_write(pp, reg, PCIE_CLIENT_INT_STATUS); >> + >> + return IRQ_HANDLED; >> +} [....] >> +static irqreturn_t rockchip_pcie_legacy_int_handler(int irq, void *arg) >> +{ >> + struct rockchip_pcie_port *pp = arg; >> + u32 reg; >> + >> + reg = pcie_read(pp, PCIE_CLIENT_INT_STATUS); >> + reg = (reg & ROCKCHIP_PCIE_RPIFR1_INTR_MASK) >> >> + ROCKCHIP_PCIE_RPIFR1_INTR_SHIFT; >> + generic_handle_irq(irq_find_mapping(pp->irq_domain, ffs(reg))); > > NAK. What you have here is a chained interrupt controller, please > implement it as such. Your mean is use handle_nested_irq instead of generic_handle_irq here? But, I found all other pci host controller drivers use this api. > >> + >> + pcie_write(pp, reg, PCIE_CLIENT_INT_STATUS); >> + return IRQ_HANDLED; >> +} >> + [...] >> +static struct platform_driver rockchip_pcie_driver = { >> + .driver = { >> + .name = "rockchip-pcie", >> + .of_match_table = rockchip_pcie_of_match, >> + .suppress_bind_attrs = true, >> + }, >> + .probe = rockchip_pcie_probe, >> + .remove = rockchip_pcie_remove, >> +}; >> +module_platform_driver(rockchip_pcie_driver); >> + >> +MODULE_AUTHOR("Rockchip Inc"); >> +MODULE_DESCRIPTION("Rockchip AXI PCIe driver"); >> +MODULE_LICENSE("GPL v2"); >> > > This definitely requires some rework for both the interrupt and MSI > parts. I'll leave Lorenzo to comment on the PCI side of things. > > Thanks, > > M. > Best Regards, Li -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01/06/16 03:56, Wenrui Li wrote: > Hi: > > On 2016/5/27 20:25, Marc Zyngier Wrote: >> [+Lorenzo] >> >> On 20/05/16 11:29, Shawn Lin wrote: >>> RK3399 has a PCIe controller which can be used as Root Complex. >>> This driver supports a PCIe controller as Root Complex mode. >>> > > [....] > >>> +static int rockchip_pcie_init_irq_domain(struct rockchip_pcie_port *pp) >>> +{ >>> + struct device *dev = pp->dev; >>> + struct device_node *node = dev->of_node; >>> + struct device_node *pcie_intc_node = of_get_next_child(node, NULL); >> >> That's really ugly, as it depends on the layout of your DT. >> >>> + >>> + if (!pcie_intc_node) { >>> + dev_err(dev, "No PCIe Intc node found\n"); >>> + return PTR_ERR(pcie_intc_node); >>> + } >>> + pp->irq_domain = irq_domain_add_linear(pcie_intc_node, 4, >>> + &intx_domain_ops, pp); >> >> Why can't you just register your host controller as the interrupt >> controller? You don't need an intermediate node for that. > > OK, the child node is really no need here, we will use the host > controller as interrupt controller next patch. Thanks! > >> >>> + if (!pp->irq_domain) { >>> + dev_err(dev, "Failed to get a INTx IRQ domain\n"); >>> + return PTR_ERR(pp->irq_domain); >>> + } >>> + >>> + return 0; >>> +} >>> + >>> +static irqreturn_t rockchip_pcie_subsys_irq_handler(int irq, void *arg) >>> +{ >>> + struct rockchip_pcie_port *pp = arg; >>> + u32 reg; >>> + u32 sub_reg; >>> + >>> + reg = pcie_read(pp, PCIE_CLIENT_INT_STATUS); >>> + if (reg & LOC_INT) { >>> + dev_dbg(pp->dev, "local interrupt recived\n"); >>> + sub_reg = pcie_read(pp, PCIE_CORE_INT_STATUS); >>> + if (sub_reg & PRFPE) >>> + dev_dbg(pp->dev, "Parity error detected while reading from the PNP Receive FIFO RAM\n"); >>> + >>> + if (sub_reg & CRFPE) >>> + dev_dbg(pp->dev, "Parity error detected while reading from the Completion Receive FIFO RAM\n"); >>> + >>> + if (sub_reg & RRPE) >>> + dev_dbg(pp->dev, "Parity error detected while reading from Replay Buffer RAM\n"); >>> + >>> + if (sub_reg & PRFO) >>> + dev_dbg(pp->dev, "Overflow occurred in the PNP Receive FIFO\n"); >>> + >>> + if (sub_reg & CRFO) >>> + dev_dbg(pp->dev, "Overflow occurred in the Completion Receive FIFO\n"); >>> + >>> + if (sub_reg & RT) >>> + dev_dbg(pp->dev, "Replay timer timed out\n"); >>> + >>> + if (sub_reg & RTR) >>> + dev_dbg(pp->dev, "Replay timer rolled over after 4 transmissions of the same TLP\n"); >>> + >>> + if (sub_reg & PE) >>> + dev_dbg(pp->dev, "Phy error detected on receive side\n"); >>> + >>> + if (sub_reg & MTR) >>> + dev_dbg(pp->dev, "Malformed TLP received from the link\n"); >>> + >>> + if (sub_reg & UCR) >>> + dev_dbg(pp->dev, "Malformed TLP received from the link\n"); >>> + >>> + if (sub_reg & FCE) >>> + dev_dbg(pp->dev, "An error was observed in the flow control advertisements from the other side\n"); >>> + >>> + if (sub_reg & CT) >>> + dev_dbg(pp->dev, "A request timed out waiting for completion\n"); >>> + >>> + if (sub_reg & UTC) >>> + dev_dbg(pp->dev, "Unmapped TC error\n"); >>> + >>> + if (sub_reg & MMVC) >>> + dev_dbg(pp->dev, "MSI mask register changes\n"); >>> + >>> + pcie_write(pp, sub_reg, PCIE_CORE_INT_STATUS); >>> + } >>> + >>> + pcie_write(pp, reg, PCIE_CLIENT_INT_STATUS); >>> + >>> + return IRQ_HANDLED; >>> +} > > [....] > >>> +static irqreturn_t rockchip_pcie_legacy_int_handler(int irq, void *arg) >>> +{ >>> + struct rockchip_pcie_port *pp = arg; >>> + u32 reg; >>> + >>> + reg = pcie_read(pp, PCIE_CLIENT_INT_STATUS); >>> + reg = (reg & ROCKCHIP_PCIE_RPIFR1_INTR_MASK) >> >>> + ROCKCHIP_PCIE_RPIFR1_INTR_SHIFT; >>> + generic_handle_irq(irq_find_mapping(pp->irq_domain, ffs(reg))); >> >> NAK. What you have here is a chained interrupt controller, please >> implement it as such. > > Your mean is use handle_nested_irq instead of generic_handle_irq here? No, handle_nested_irq() is for threaded interrupts. What I mean is that you need to configure the interrupt as a cascaded interrupt, and use the chained_irq_enter/exit helpers. As a rule of thumb, if you're calling generic_handle_irq() in an interrupt handler, you're doing something wrong. If you don't do that, you may end up with interrupts that are not EOIed properly, RCU violations, and double accounting of interrupts. > But, I found all other pci host controller drivers use this api. I'm afraid that doesn't make it any better. Buggy code is everywhere, and it does spread faster than properly written code. Have a look at drivers/pci/host/pcie-altera.c as an example of what it should look like. Thanks, M.
On Fri, May 27, 2016 at 01:25:14PM +0100, Marc Zyngier wrote: [...] > > + } else { > > + bus = pci_scan_root_bus(&pdev->dev, 0, > > + &rockchip_pcie_ops, port, &res); > > + } > > + if (!bus) > > + return -ENOMEM; > > + > > + if (!pci_has_flag(PCI_PROBE_ONLY)) { > > Why do you have catter for the PCI_PROBE_ONLY case? Nobody should ever > use that for properly implemented HW. I think that's just copy and paste and it is a useless check given that the only way we can set that flag on ARM/ARM64 is through DT (of_pci_check_probe_only()) and I doubt that systems probing this driver really require a PCI_PROBE_ONLY set-up. So, unless you can explain to us why it is really needed, please remove the: if (!pci_has_flag(PCI_PROBE_ONLY)) check. Lorenzo -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 03/06/16 09:55, Lorenzo Pieralisi wrote: > On Fri, May 27, 2016 at 01:25:14PM +0100, Marc Zyngier wrote: > > [...] > >>> + } else { >>> + bus = pci_scan_root_bus(&pdev->dev, 0, >>> + &rockchip_pcie_ops, port, &res); >>> + } >>> + if (!bus) >>> + return -ENOMEM; >>> + >>> + if (!pci_has_flag(PCI_PROBE_ONLY)) { >> >> Why do you have catter for the PCI_PROBE_ONLY case? Nobody should ever >> use that for properly implemented HW. > > I think that's just copy and paste and it is a useless check given > that the only way we can set that flag on ARM/ARM64 is through DT > (of_pci_check_probe_only()) and I doubt that systems probing this > driver really require a PCI_PROBE_ONLY set-up. > > So, unless you can explain to us why it is really needed, please > remove the: > > if (!pci_has_flag(PCI_PROBE_ONLY)) > > check. Agreed. Maybe we should add a comment somewhere saying that this is deprecated on arm/arm64, and only useful to slightly dumb virtualization environments (kvmtool being the prime example). Thanks, M.
diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig index 8e4f038..76447a8 100644 --- a/drivers/pci/host/Kconfig +++ b/drivers/pci/host/Kconfig @@ -245,4 +245,16 @@ config PCIE_ARMADA_8K Designware hardware and therefore the driver re-uses the Designware core functions to implement the driver. +config PCIE_ROCKCHIP + bool "Rockchip PCIe controller" + depends on ARM64 && ARCH_ROCKCHIP + depends on OF + select MFD_SYSCON + select PCI_MSI + select PCI_MSI_IRQ_DOMAIN + help + Say Y here if you want internal PCI support on Rockchip SoC. + There are 1 internal PCIe port available to support GEN2 with + 4 slots. + endmenu diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile index a6f85e3..fb3871e 100644 --- a/drivers/pci/host/Makefile +++ b/drivers/pci/host/Makefile @@ -29,3 +29,4 @@ obj-$(CONFIG_PCIE_QCOM) += pcie-qcom.o obj-$(CONFIG_PCI_HOST_THUNDER_ECAM) += pci-thunder-ecam.o obj-$(CONFIG_PCI_HOST_THUNDER_PEM) += pci-thunder-pem.o obj-$(CONFIG_PCIE_ARMADA_8K) += pcie-armada8k.o +obj-$(CONFIG_PCIE_ROCKCHIP) += pcie-rockchip.o diff --git a/drivers/pci/host/pcie-rockchip.c b/drivers/pci/host/pcie-rockchip.c new file mode 100644 index 0000000..4063fd3 --- /dev/null +++ b/drivers/pci/host/pcie-rockchip.c @@ -0,0 +1,1181 @@ +/* + * Rockchip AXI PCIe host controller driver + * + * Copyright (c) 2016 Rockchip, Inc. + * + * Author: Shawn Lin <shawn.lin@rock-chips.com> + * Wenrui Li <wenrui.li@rock-chips.com> + * Bits taken from Synopsys Designware Host controller driver and + * ARM PCI Host generic driver. + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 2 of the License, or + * (at your option) any later version. + */ +#include <linux/clk.h> +#include <linux/delay.h> +#include <linux/gpio/consumer.h> +#include <linux/interrupt.h> +#include <linux/irq.h> +#include <linux/irqdomain.h> +#include <linux/kernel.h> +#include <linux/mfd/syscon.h> +#include <linux/module.h> +#include <linux/msi.h> +#include <linux/of_address.h> +#include <linux/of_device.h> +#include <linux/of_pci.h> +#include <linux/of_platform.h> +#include <linux/of_irq.h> +#include <linux/pci.h> +#include <linux/platform_device.h> +#include <linux/reset.h> +#include <linux/regmap.h> + +#define REF_CLK_100MHZ (100 * 1000 * 1000) +#define PCIE_CLIENT_BASE 0x0 +#define PCIE_RC_CONFIG_BASE 0xa00000 +#define PCIE_CORE_CTRL_MGMT_BASE 0x900000 +#define PCIE_CORE_AXI_CONF_BASE 0xc00000 +#define PCIE_CORE_AXI_INBOUND_BASE 0xc00800 + +#define PCIE_CLIENT_BASIC_STATUS0 0x44 +#define PCIE_CLIENT_BASIC_STATUS1 0x48 +#define PCIE_CLIENT_INT_MASK 0x4c +#define PCIE_CLIENT_INT_STATUS 0x50 +#define PCIE_CORE_INT_MASK 0x900210 +#define PCIE_CORE_INT_STATUS 0x90020c + +/** Size of one AXI Region (not Region 0) */ +#define AXI_REGION_SIZE (0x1 << 20) +/** Overall size of AXI area */ +#define AXI_OVERALL_SIZE (64 * (0x1 << 20)) +/** Size of Region 0, equal to sum of sizes of other regions */ +#define AXI_REGION_0_SIZE (32 * (0x1 << 20)) +#define OB_REG_SIZE_SHIFT 5 +#define IB_ROOT_PORT_REG_SIZE_SHIFT 3 + +#define AXI_WRAPPER_IO_WRITE 0x6 +#define AXI_WRAPPER_MEM_WRITE 0x2 +#define MAX_AXI_IB_ROOTPORT_REGION_NUM 3 +#define MIN_AXI_ADDR_BITS_PASSED 8 + +#define ROCKCHIP_PCIE_RPIFR1_INTR_MASK GENMASK(8, 5) +#define ROCKCHIP_PCIE_RPIFR1_INTR_SHIFT 5 +#define CLIENT_INTERRUPTS \ + (LOC_INT | INTA | INTB | INTC | INTD |\ + CORR_ERR | NFATAL_ERR | FATAL_ERR | DPA_INT | \ + HOT_RESET | MSG_DONE | LEGACY_DONE) +#define CORE_INTERRUPTS \ + (PRFPE | CRFPE | RRPE | CRFO | RT | RTR | \ + PE | MTR | UCR | FCE | CT | UTC | MMVC) +#define PWR_STCG BIT(0) +#define HOT_PLUG BIT(1) +#define PHY_INT BIT(2) +#define UDMA_INT BIT(3) +#define LOC_INT BIT(4) +#define INTA BIT(5) +#define INTB BIT(6) +#define INTC BIT(7) +#define INTD BIT(8) +#define CORR_ERR BIT(9) +#define NFATAL_ERR BIT(10) +#define FATAL_ERR BIT(11) +#define DPA_INT BIT(12) +#define HOT_RESET BIT(13) +#define MSG_DONE BIT(14) +#define LEGACY_DONE BIT(15) +#define PRFPE BIT(0) +#define CRFPE BIT(1) +#define RRPE BIT(2) +#define PRFO BIT(3) +#define CRFO BIT(4) +#define RT BIT(5) +#define RTR BIT(6) +#define PE BIT(7) +#define MTR BIT(8) +#define UCR BIT(9) +#define FCE BIT(10) +#define CT BIT(11) +#define UTC BIT(18) +#define MMVC BIT(19) + +#define PCIE_ECAM_BUS(x) (((x) & 0xFF) << 20) +#define PCIE_ECAM_DEV(x) (((x) & 0x1F) << 15) +#define PCIE_ECAM_FUNC(x) (((x) & 0x7) << 12) +#define PCIE_ECAM_REG(x) (((x) & 0xFFF) << 0) +#define PCIE_ECAM_ADDR(bus, dev, func, reg) \ + (PCIE_ECAM_BUS(bus) | PCIE_ECAM_DEV(dev) | \ + PCIE_ECAM_FUNC(func) | PCIE_ECAM_REG(reg)) + +#define RC_REGION_0_ADDR_TRANS_H 0x00000000 +#define RC_REGION_0_ADDR_TRANS_L 0x00000000 +#define RC_REGION_0_PASS_BITS (25 - 1) +#define RC_REGION_1_ADDR_TRANS_H 0x00000000 +#define RC_REGION_1_ADDR_TRANS_L 0x00400000 +#define RC_REGION_1_PASS_BITS (20 - 1) +#define MAX_AXI_WRAPPER_REGION_NUM 33 +#define PCIE_CLIENT_CONF_ENABLE BIT(0) +#define PCIE_CLIENT_CONF_LANE_NUM(x) ((x / 2) << 4) +#define PCIE_CLIENT_MODE_RC BIT(6) +#define PCIE_CLIENT_GEN_SEL_2 BIT(7) +#define PCIE_CLIENT_GEN_SEL_1 0x0 + +struct rockchip_pcie_port { + void __iomem *reg_base; + void __iomem *apb_base; + struct regmap *grf; + unsigned int pcie_conf; + unsigned int pcie_status; + unsigned int pcie_laneoff; + struct reset_control *phy_rst; + struct reset_control *core_rst; + struct reset_control *mgmt_rst; + struct reset_control *mgmt_sticky_rst; + struct reset_control *pipe_rst; + struct clk *aclk_pcie; + struct clk *aclk_perf_pcie; + struct clk *hclk_pcie; + struct clk *clk_pciephy_ref; + struct gpio_desc *ep_gpio; + u32 lanes; + resource_size_t io_base; + struct resource *cfg; + struct resource *io; + struct resource *mem; + struct resource *busn; + phys_addr_t io_bus_addr; + u32 io_size; + phys_addr_t mem_bus_addr; + u32 mem_size; + u8 root_bus_nr; + int irq; + struct msi_controller *msi; + + struct device *dev; + struct irq_domain *irq_domain; +}; + +static inline u32 pcie_read(struct rockchip_pcie_port *port, u32 reg) +{ + return readl(port->apb_base + reg); +} + +static inline void pcie_write(struct rockchip_pcie_port *port, + u32 val, u32 reg) +{ + writel(val, port->apb_base + reg); +} + +static inline void pcie_pb_wr_cfg(struct rockchip_pcie_port *port, + u32 addr, u32 data) +{ + regmap_write(port->grf, port->pcie_conf, + (0x3ff << 17) | (data << 7) | (addr << 1)); + udelay(1); + regmap_write(port->grf, port->pcie_conf, + (0x1 << 16) | (0x1 << 0)); + udelay(1); + regmap_write(port->grf, port->pcie_conf, + (0x1 << 16) | (0x0 << 0)); +} + +static inline u32 pcie_pb_rd_cfg(struct rockchip_pcie_port *port, + u32 addr) +{ + u32 val; + + regmap_write(port->grf, port->pcie_conf, + (0x3ff << 17) | (addr << 1)); + regmap_read(port->grf, port->pcie_status, &val); + return val; +} + +static int rockchip_pcie_valid_config(struct rockchip_pcie_port *pp, + struct pci_bus *bus, int dev) +{ + /* access only one slot on each root port */ + if (bus->number == pp->root_bus_nr && dev > 0) + return 0; + + /* + * do not read more than one device on the bus directly attached + * to RC's (Virtual Bridge's) DS side. + */ + if (bus->primary == pp->root_bus_nr && dev > 0) + return 0; + + return 1; +} + +static int rockchip_pcie_rd_own_conf(struct rockchip_pcie_port *pp, + int where, int size, + u32 *val) +{ + void __iomem *addr = pp->apb_base + PCIE_RC_CONFIG_BASE + where; + + if ((uintptr_t)addr & (size - 1)) { + *val = 0; + return PCIBIOS_BAD_REGISTER_NUMBER; + } + + if (size == 4) { + *val = readl(addr); + } else if (size == 2) { + *val = readw(addr); + } else if (size == 1) { + *val = readb(addr); + } else { + *val = 0; + return PCIBIOS_BAD_REGISTER_NUMBER; + } + return PCIBIOS_SUCCESSFUL; +} + +static int rockchip_pcie_wr_own_conf(struct rockchip_pcie_port *pp, + int where, int size, u32 val) +{ + u32 tmp; + int offset; + + offset = (where & (~0x3)); + tmp = readl(pp->apb_base + PCIE_RC_CONFIG_BASE + offset); + if (size == 4) { + writel(val, pp->apb_base + PCIE_RC_CONFIG_BASE + where); + } else if (size == 2) { + if (where & 0x2) + tmp = ((tmp & 0xffff) | (val << 16)); + else + tmp = ((tmp & 0xffff0000) | val); + + writel(tmp, pp->apb_base + PCIE_RC_CONFIG_BASE + offset); + } else if (size == 1) { + if ((where & 0x3) == 0) + tmp = ((tmp & (~0xff)) | val); + else if ((where & 0x3) == 1) + tmp = ((tmp & (~0xff00)) | (val << 8)); + else if ((where & 0x3) == 2) + tmp = ((tmp & (~0xff0000)) | (val << 16)); + else if ((where & 0x3) == 3) + tmp = ((tmp & (~0xff000000)) | (val << 24)); + + writel(tmp, pp->apb_base + PCIE_RC_CONFIG_BASE + offset); + } else { + return PCIBIOS_BAD_REGISTER_NUMBER; + } + return PCIBIOS_SUCCESSFUL; +} + +static int rockchip_pcie_rd_other_conf(struct rockchip_pcie_port *pp, + struct pci_bus *bus, u32 devfn, + int where, int size, u32 *val) +{ + u32 busdev; + + busdev = PCIE_ECAM_ADDR(bus->number, PCI_SLOT(devfn), + PCI_FUNC(devfn), where); + + if (busdev & (size - 1)) { + *val = 0; + return PCIBIOS_BAD_REGISTER_NUMBER; + } + + if (size == 4) { + *val = readl(pp->reg_base + busdev); + } else if (size == 2) { + *val = readw(pp->reg_base + busdev); + } else if (size == 1) { + *val = readb(pp->reg_base + busdev); + } else { + *val = 0; + return PCIBIOS_BAD_REGISTER_NUMBER; + } + return PCIBIOS_SUCCESSFUL; +} + +static int rockchip_pcie_wr_other_conf(struct rockchip_pcie_port *pp, + struct pci_bus *bus, u32 devfn, + int where, int size, u32 val) +{ + u32 busdev; + + busdev = PCIE_ECAM_ADDR(bus->number, PCI_SLOT(devfn), + PCI_FUNC(devfn), where); + if (busdev & (size - 1)) + return PCIBIOS_BAD_REGISTER_NUMBER; + + if (size == 4) + writel(val, pp->reg_base + busdev); + else if (size == 2) + writew(val, pp->reg_base + busdev); + else if (size == 1) + writeb(val, pp->reg_base + busdev); + else + return PCIBIOS_BAD_REGISTER_NUMBER; + + return PCIBIOS_SUCCESSFUL; +} + +static int rockchip_pcie_rd_conf(struct pci_bus *bus, u32 devfn, int where, + int size, u32 *val) +{ + struct rockchip_pcie_port *pp = bus->sysdata; + int ret; + + if (rockchip_pcie_valid_config(pp, bus, PCI_SLOT(devfn)) == 0) { + *val = 0xffffffff; + return PCIBIOS_DEVICE_NOT_FOUND; + } + + if (bus->number != pp->root_bus_nr) + ret = rockchip_pcie_rd_other_conf(pp, bus, devfn, + where, size, val); + else + ret = rockchip_pcie_rd_own_conf(pp, where, size, val); + + return ret; +} + +static int rockchip_pcie_wr_conf(struct pci_bus *bus, u32 devfn, + int where, int size, u32 val) +{ + struct rockchip_pcie_port *pp = bus->sysdata; + int ret; + + if (rockchip_pcie_valid_config(pp, bus, PCI_SLOT(devfn)) == 0) + return PCIBIOS_DEVICE_NOT_FOUND; + + if (bus->number != pp->root_bus_nr) + ret = rockchip_pcie_wr_other_conf(pp, bus, devfn, + where, size, val); + else + ret = rockchip_pcie_wr_own_conf(pp, where, size, val); + + return ret; +} + +static struct pci_ops rockchip_pcie_ops = { + .read = rockchip_pcie_rd_conf, + .write = rockchip_pcie_wr_conf, +}; + +/** + * rockchip_pcie_init_port - Initialize hardware + * @port: PCIe port information + */ +static int rockchip_pcie_init_port(struct rockchip_pcie_port *port) +{ + int err; + u32 status; + unsigned long timeout = jiffies + msecs_to_jiffies(1000); + + gpiod_set_value(port->ep_gpio, 0); + + /* Make sure PCIe relate block is in reset state */ + err = reset_control_assert(port->phy_rst); + if (err) { + dev_err(port->dev, "assert phy_rst err %d\n", err); + return err; + } + err = reset_control_assert(port->core_rst); + if (err) { + dev_err(port->dev, "assert core_rst err %d\n", err); + return err; + } + err = reset_control_assert(port->mgmt_rst); + if (err) { + dev_err(port->dev, "assert mgmt_rst err %d\n", err); + return err; + } + err = reset_control_assert(port->mgmt_sticky_rst); + if (err) { + dev_err(port->dev, "assert mgmt_sticky_rst err %d\n", err); + return err; + } + err = reset_control_assert(port->pipe_rst); + if (err) { + dev_err(port->dev, "assert pipe_rst err %d\n", err); + return err; + } + + pcie_write(port, (0xf << 20) | (0x1 << 16) | PCIE_CLIENT_GEN_SEL_2 | + (0x1 << 19) | (0x1 << 3) | + PCIE_CLIENT_MODE_RC | + PCIE_CLIENT_CONF_LANE_NUM(port->lanes) | + PCIE_CLIENT_CONF_ENABLE, PCIE_CLIENT_BASE); + + err = reset_control_deassert(port->phy_rst); + if (err) { + dev_err(port->dev, "deassert phy_rst err %d\n", err); + return err; + } + regmap_write(port->grf, port->pcie_conf, + (0x3f << 17) | (0x10 << 1)); + err = -EINVAL; + while (time_before(jiffies, timeout)) { + regmap_read(port->grf, port->pcie_status, &status); + if ((status & (1 << 9))) { + dev_info(port->dev, "pll locked!\n"); + err = 0; + break; + } + } + if (err) { + dev_err(port->dev, "pll lock timeout!\n"); + return err; + } + pcie_pb_wr_cfg(port, 0x10, 0x8); + pcie_pb_wr_cfg(port, 0x12, 0x8); + + err = -ETIMEDOUT; + while (time_before(jiffies, timeout)) { + regmap_read(port->grf, port->pcie_status, &status); + if (!(status & (1 << 10))) { + dev_info(port->dev, "pll output enable done!\n"); + err = 0; + break; + } + } + + if (err) { + dev_err(port->dev, "pll output enable timeout!\n"); + return err; + } + + regmap_write(port->grf, port->pcie_conf, + (0x3f << 17) | (0x10 << 1)); + err = -EINVAL; + while (time_before(jiffies, timeout)) { + regmap_read(port->grf, port->pcie_status, &status); + if ((status & (1 << 9))) { + dev_info(port->dev, "pll relocked!\n"); + err = 0; + break; + } + } + if (err) { + dev_err(port->dev, "pll relock timeout!\n"); + return err; + } + + err = reset_control_deassert(port->core_rst); + if (err) { + dev_err(port->dev, "deassert core_rst err %d\n", err); + return err; + } + err = reset_control_deassert(port->mgmt_rst); + if (err) { + dev_err(port->dev, "deassert mgmt_rst err %d\n", err); + return err; + } + err = reset_control_deassert(port->mgmt_sticky_rst); + if (err) { + dev_err(port->dev, "deassert mgmt_sticky_rst err %d\n", err); + return err; + } + err = reset_control_deassert(port->pipe_rst); + if (err) { + dev_err(port->dev, "deassert pipe_rst err %d\n", err); + return err; + } + + pcie_write(port, 1 << 17 | 1 << 1, PCIE_CLIENT_BASE); + + gpiod_set_value(port->ep_gpio, 1); + err = -ETIMEDOUT; + while (time_before(jiffies, timeout)) { + status = pcie_read(port, PCIE_CLIENT_BASIC_STATUS1); + if (((status >> 20) & 0x3) == 0x3) { + dev_info(port->dev, "pcie link training gen1 pass!\n"); + err = 0; + break; + } + } + if (err) { + dev_err(port->dev, "pcie link training gen1 timeout!\n"); + return err; + } + + status = pcie_read(port, 0x9000d0); + status |= 0x20; + pcie_write(port, status, 0x9000d0); + err = -ETIMEDOUT; + while (time_before(jiffies, timeout)) { + status = pcie_read(port, PCIE_CORE_CTRL_MGMT_BASE); + if (((status >> 3) & 0x3) == 0x1) { + dev_info(port->dev, "pcie link training gen2 pass!\n"); + err = 0; + break; + } + } + if (err) + dev_dbg(port->dev, "pcie link training gen2 timeout, force to gen1!\n"); + + if (((status >> 3) & 0x3) == 0x0) + dev_info(port->dev, "pcie link 2.5!\n"); + if (((status >> 3) & 0x3) == 0x1) + dev_info(port->dev, "pcie link 5.0!\n"); + + status = pcie_read(port, PCIE_CORE_CTRL_MGMT_BASE); + status = 0x1 << ((status >> 1) & 0x3); + dev_info(port->dev, "current link width is x%d\n", status); + + status = pcie_pb_rd_cfg(port, 0x30); + if (!((status >> 11) & 0x1)) + dev_dbg(port->dev, "lane A is used\n"); + else + regmap_write(port->grf, port->pcie_laneoff, + (0x1 << 19) | (0x1 << 3)); + + status = pcie_pb_rd_cfg(port, 0x31); + if (!((status >> 11) & 0x1)) + dev_dbg(port->dev, "lane B is used\n"); + else + regmap_write(port->grf, port->pcie_laneoff, + (0x2 << 19) | (0x2 << 3)); + + status = pcie_pb_rd_cfg(port, 0x32); + if (!((status >> 11) & 0x1)) + dev_dbg(port->dev, "lane C is used\n"); + else + regmap_write(port->grf, port->pcie_laneoff, + (0x4 << 19) | (0x4 << 3)); + + status = pcie_pb_rd_cfg(port, 0x33); + if (!((status >> 11) & 0x1)) + dev_dbg(port->dev, "lane D is used\n"); + else + regmap_write(port->grf, port->pcie_laneoff, + (0x8 << 19) | (0x8 << 3)); + return 0; +} + +/** + * rockchip_pcie_parse_dt - Parse Device tree + * @port: PCIe port information + * + * Return: '0' on success and error value on failure + */ +static int rockchip_pcie_parse_dt(struct rockchip_pcie_port *port) +{ + struct device *dev = port->dev; + struct device_node *node = dev->of_node; + struct resource regs; + unsigned int pcie_conf; + unsigned int pcie_status; + unsigned int pcie_laneoff; + int err; + + err = of_address_to_resource(node, 0, ®s); + if (err) { + dev_err(dev, "missing \"reg\" property\n"); + return err; + } + + port->reg_base = devm_ioremap_resource(dev, ®s); + if (IS_ERR(port->reg_base)) + return PTR_ERR(port->reg_base); + + err = of_address_to_resource(node, 1, ®s); + if (err) { + dev_err(dev, "missing \"reg\" property\n"); + return err; + } + + port->apb_base = devm_ioremap_resource(dev, ®s); + if (IS_ERR(port->apb_base)) + return PTR_ERR(port->apb_base); + + port->grf = syscon_regmap_lookup_by_phandle(dev->of_node, + "rockchip,grf"); + if (IS_ERR(port->grf)) { + dev_err(dev, "Missing rockchip,grf property\n"); + return PTR_ERR(port->grf); + } + + if (of_property_read_u32(node, "pcie-conf", &pcie_conf)) { + dev_err(dev, "missing pcie-conf property in node %s\n", + node->name); + return -EINVAL; + } + + port->pcie_conf = pcie_conf; + + if (of_property_read_u32(node, "pcie-status", &pcie_status)) { + dev_err(dev, "missing pcie-status property in node %s\n", + node->name); + return -EINVAL; + } + + port->pcie_status = pcie_status; + + if (of_property_read_u32(node, "pcie-laneoff", &pcie_laneoff)) { + dev_err(dev, "missing pcie-laneoff property in node %s\n", + node->name); + return -EINVAL; + } + + port->pcie_laneoff = pcie_laneoff; + + port->lanes = 1; + err = of_property_read_u32(node, "num-lanes", &port->lanes); + if (!err && ((port->lanes == 0) || + (port->lanes == 3) || + (port->lanes > 4))) { + dev_info(dev, "invalid num-lanes, default use one lane\n"); + port->lanes = 1; + } + + port->phy_rst = devm_reset_control_get(dev, "phy-rst"); + if (IS_ERR(port->phy_rst)) { + if (PTR_ERR(port->phy_rst) != -EPROBE_DEFER) + dev_err(dev, "missing phy-rst property in node %s\n", + node->name); + err = PTR_ERR(port->phy_rst); + goto err_aclk_pcie; + } + + port->core_rst = devm_reset_control_get(dev, "core-rst"); + if (IS_ERR(port->core_rst)) { + if (PTR_ERR(port->core_rst) != -EPROBE_DEFER) + dev_err(dev, "missing core-rst property in node %s\n", + node->name); + err = PTR_ERR(port->core_rst); + goto err_aclk_pcie; + } + + port->mgmt_rst = devm_reset_control_get(dev, "mgmt-rst"); + if (IS_ERR(port->mgmt_rst)) { + if (PTR_ERR(port->mgmt_rst) != -EPROBE_DEFER) + dev_err(dev, "missing mgmt-rst property in node %s\n", + node->name); + err = PTR_ERR(port->mgmt_rst); + goto err_aclk_pcie; + } + + port->mgmt_sticky_rst = devm_reset_control_get(dev, "mgmt-sticky-rst"); + if (IS_ERR(port->mgmt_sticky_rst)) { + if (PTR_ERR(port->mgmt_sticky_rst) != -EPROBE_DEFER) + dev_err(dev, "missing mgmt-sticky-rst property in node %s\n", + node->name); + err = PTR_ERR(port->mgmt_sticky_rst); + goto err_aclk_pcie; + } + + port->pipe_rst = devm_reset_control_get(dev, "pipe-rst"); + if (IS_ERR(port->pipe_rst)) { + if (PTR_ERR(port->pipe_rst) != -EPROBE_DEFER) + dev_err(dev, "missing pipe-rst property in node %s\n", + node->name); + err = PTR_ERR(port->pipe_rst); + goto err_aclk_pcie; + } + + port->ep_gpio = gpiod_get(dev, "ep", GPIOD_OUT_HIGH); + if (IS_ERR(port->ep_gpio)) { + dev_err(dev, "missing ep-gpios property in node %s\n", + node->name); + return PTR_ERR(port->ep_gpio); + } + + port->aclk_pcie = devm_clk_get(dev, "aclk_pcie"); + if (IS_ERR(port->aclk_pcie)) { + dev_err(dev, "aclk_pcie clock not found.\n"); + return PTR_ERR(port->aclk_pcie); + } + + port->aclk_perf_pcie = devm_clk_get(dev, "aclk_perf_pcie"); + if (IS_ERR(port->aclk_perf_pcie)) { + dev_err(dev, "aclk_perf_pcie clock not found.\n"); + return PTR_ERR(port->aclk_perf_pcie); + } + + port->hclk_pcie = devm_clk_get(dev, "hclk_pcie"); + if (IS_ERR(port->hclk_pcie)) { + dev_err(dev, "hclk_pcie clock not found.\n"); + return PTR_ERR(port->hclk_pcie); + } + + port->clk_pciephy_ref = devm_clk_get(dev, "clk_pciephy_ref"); + if (IS_ERR(port->clk_pciephy_ref)) { + dev_err(dev, "clk_pciephy_ref clock not found.\n"); + return PTR_ERR(port->clk_pciephy_ref); + } + + err = clk_prepare_enable(port->aclk_pcie); + if (err) { + dev_err(dev, "Unable to enable aclk_pcie clock.\n"); + goto err_aclk_pcie; + } + + err = clk_prepare_enable(port->aclk_perf_pcie); + if (err) { + dev_err(dev, "Unable to enable aclk_perf_pcie clock.\n"); + goto err_aclk_perf_pcie; + } + + err = clk_prepare_enable(port->hclk_pcie); + if (err) { + dev_err(dev, "Unable to enable hclk_pcie clock.\n"); + goto err_hclk_pcie; + } + + err = clk_prepare_enable(port->clk_pciephy_ref); + if (err) { + dev_err(dev, "Unable to enable hclk_pcie clock.\n"); + goto err_pciephy_ref; + } + + return 0; + +err_pciephy_ref: + clk_disable_unprepare(port->hclk_pcie); +err_hclk_pcie: + clk_disable_unprepare(port->aclk_perf_pcie); +err_aclk_perf_pcie: + clk_disable_unprepare(port->aclk_pcie); +err_aclk_pcie: + return err; +} + +static void rockchip_pcie_msi_enable(struct rockchip_pcie_port *pp) +{ + struct device_node *msi_node; + + msi_node = of_parse_phandle(pp->dev->of_node, + "msi-parent", 0); + if (!msi_node) + return; + + pp->msi = of_pci_find_msi_chip_by_node(msi_node); + of_node_put(msi_node); + + if (pp->msi) + pp->msi->dev = pp->dev; +} + +static void rockchip_pcie_enable_interrupts(struct rockchip_pcie_port *pp) +{ + pcie_write(pp, (CLIENT_INTERRUPTS << 16) & + (~CLIENT_INTERRUPTS), PCIE_CLIENT_INT_MASK); + pcie_write(pp, CORE_INTERRUPTS, PCIE_CORE_INT_MASK); +} + +static int rockchip_pcie_intx_map(struct irq_domain *domain, unsigned int irq, + irq_hw_number_t hwirq) +{ + irq_set_chip_and_handler(irq, &dummy_irq_chip, handle_simple_irq); + irq_set_chip_data(irq, domain->host_data); + + return 0; +} + +static const struct irq_domain_ops intx_domain_ops = { + .map = rockchip_pcie_intx_map, +}; + +static int rockchip_pcie_init_irq_domain(struct rockchip_pcie_port *pp) +{ + struct device *dev = pp->dev; + struct device_node *node = dev->of_node; + struct device_node *pcie_intc_node = of_get_next_child(node, NULL); + + if (!pcie_intc_node) { + dev_err(dev, "No PCIe Intc node found\n"); + return PTR_ERR(pcie_intc_node); + } + pp->irq_domain = irq_domain_add_linear(pcie_intc_node, 4, + &intx_domain_ops, pp); + if (!pp->irq_domain) { + dev_err(dev, "Failed to get a INTx IRQ domain\n"); + return PTR_ERR(pp->irq_domain); + } + + return 0; +} + +static irqreturn_t rockchip_pcie_subsys_irq_handler(int irq, void *arg) +{ + struct rockchip_pcie_port *pp = arg; + u32 reg; + u32 sub_reg; + + reg = pcie_read(pp, PCIE_CLIENT_INT_STATUS); + if (reg & LOC_INT) { + dev_dbg(pp->dev, "local interrupt recived\n"); + sub_reg = pcie_read(pp, PCIE_CORE_INT_STATUS); + if (sub_reg & PRFPE) + dev_dbg(pp->dev, "Parity error detected while reading from the PNP Receive FIFO RAM\n"); + + if (sub_reg & CRFPE) + dev_dbg(pp->dev, "Parity error detected while reading from the Completion Receive FIFO RAM\n"); + + if (sub_reg & RRPE) + dev_dbg(pp->dev, "Parity error detected while reading from Replay Buffer RAM\n"); + + if (sub_reg & PRFO) + dev_dbg(pp->dev, "Overflow occurred in the PNP Receive FIFO\n"); + + if (sub_reg & CRFO) + dev_dbg(pp->dev, "Overflow occurred in the Completion Receive FIFO\n"); + + if (sub_reg & RT) + dev_dbg(pp->dev, "Replay timer timed out\n"); + + if (sub_reg & RTR) + dev_dbg(pp->dev, "Replay timer rolled over after 4 transmissions of the same TLP\n"); + + if (sub_reg & PE) + dev_dbg(pp->dev, "Phy error detected on receive side\n"); + + if (sub_reg & MTR) + dev_dbg(pp->dev, "Malformed TLP received from the link\n"); + + if (sub_reg & UCR) + dev_dbg(pp->dev, "Malformed TLP received from the link\n"); + + if (sub_reg & FCE) + dev_dbg(pp->dev, "An error was observed in the flow control advertisements from the other side\n"); + + if (sub_reg & CT) + dev_dbg(pp->dev, "A request timed out waiting for completion\n"); + + if (sub_reg & UTC) + dev_dbg(pp->dev, "Unmapped TC error\n"); + + if (sub_reg & MMVC) + dev_dbg(pp->dev, "MSI mask register changes\n"); + + pcie_write(pp, sub_reg, PCIE_CORE_INT_STATUS); + } + + pcie_write(pp, reg, PCIE_CLIENT_INT_STATUS); + + return IRQ_HANDLED; +} + +static irqreturn_t rockchip_pcie_client_irq_handler(int irq, void *arg) +{ + struct rockchip_pcie_port *pp = arg; + u32 reg; + + reg = pcie_read(pp, PCIE_CLIENT_INT_STATUS); + if (reg & LEGACY_DONE) + dev_dbg(pp->dev, "legacy done interrupt recived\n"); + + if (reg & MSG_DONE) + dev_dbg(pp->dev, "message done interrupt recived\n"); + + if (reg & HOT_RESET) + dev_dbg(pp->dev, "hot reset interrupt recived\n"); + + if (reg & DPA_INT) + dev_dbg(pp->dev, "dpa interrupt recived\n"); + + if (reg & FATAL_ERR) + dev_dbg(pp->dev, "fatal error interrupt recived\n"); + + if (reg & DPA_INT) + dev_dbg(pp->dev, "no fatal error interrupt recived\n"); + + if (reg & CORR_ERR) + dev_dbg(pp->dev, "correctable error interrupt recived\n"); + + pcie_write(pp, reg, PCIE_CLIENT_INT_STATUS); + + return IRQ_HANDLED; +} + +static irqreturn_t rockchip_pcie_legacy_int_handler(int irq, void *arg) +{ + struct rockchip_pcie_port *pp = arg; + u32 reg; + + reg = pcie_read(pp, PCIE_CLIENT_INT_STATUS); + reg = (reg & ROCKCHIP_PCIE_RPIFR1_INTR_MASK) >> + ROCKCHIP_PCIE_RPIFR1_INTR_SHIFT; + generic_handle_irq(irq_find_mapping(pp->irq_domain, ffs(reg))); + + pcie_write(pp, reg, PCIE_CLIENT_INT_STATUS); + return IRQ_HANDLED; +} + +static int rockchip_pcie_prog_ob_atu(struct rockchip_pcie_port *pp, + int region_no, + int type, u8 num_pass_bits, + u32 lower_addr, u32 upper_addr) +{ + u32 ob_addr_0 = 0; + u32 ob_addr_1 = 0; + u32 ob_desc_0 = 0; + u32 ob_desc_1 = 0; + void __iomem *aw_base; + + if (!pp) + return -EINVAL; + if (region_no >= MAX_AXI_WRAPPER_REGION_NUM) + return -EINVAL; + if ((num_pass_bits + 1) < 8) + return -EINVAL; + if (num_pass_bits > 63) + return -EINVAL; + if (region_no == 0) { + if (AXI_REGION_0_SIZE < (2ULL << num_pass_bits)) + return -EINVAL; + } + if (region_no != 0) { + if (AXI_REGION_SIZE < (2ULL << num_pass_bits)) + return -EINVAL; + } + aw_base = pp->apb_base + PCIE_CORE_AXI_CONF_BASE; + aw_base += (region_no << OB_REG_SIZE_SHIFT); + + ob_addr_0 = (ob_addr_0 & + ~0x0000003fU) | (num_pass_bits & + 0x0000003fU); + ob_addr_0 = (ob_addr_0 & + ~0xffffff00U) | (lower_addr & 0xffffff00U); + ob_addr_1 = upper_addr; + ob_desc_0 = (1 << 23 | type); + + writel(ob_addr_0, aw_base); + writel(ob_addr_1, aw_base + 0x4); + writel(ob_desc_0, aw_base + 0x8); + writel(ob_desc_1, aw_base + 0xc); + + return 0; +} + +static int rockchip_pcie_prog_ib_atu(struct rockchip_pcie_port *pp, + int region_no, + u8 num_pass_bits, + u32 lower_addr, + u32 upper_addr) +{ + u32 ib_addr_0 = 0; + u32 ib_addr_1 = 0; + void __iomem *aw_base; + + if (!pp) + return -EINVAL; + if (region_no > MAX_AXI_IB_ROOTPORT_REGION_NUM) + return -EINVAL; + if ((num_pass_bits + 1) < MIN_AXI_ADDR_BITS_PASSED) + return -EINVAL; + if (num_pass_bits > 63) + return -EINVAL; + aw_base = pp->apb_base + PCIE_CORE_AXI_INBOUND_BASE; + aw_base += (region_no << IB_ROOT_PORT_REG_SIZE_SHIFT); + ib_addr_0 = (ib_addr_0 & + ~0x0000003fU) | (num_pass_bits & + 0x0000003fU); + + ib_addr_0 = (ib_addr_0 & ~0xffffff00U) | + ((lower_addr << 8) & 0xffffff00U); + ib_addr_1 = upper_addr; + writel(ib_addr_0, aw_base); + writel(ib_addr_1, aw_base + 0x4); + + return 0; +} + +static int rockchip_pcie_probe(struct platform_device *pdev) +{ + struct rockchip_pcie_port *port; + struct device *dev = &pdev->dev; + struct pci_bus *bus, *child; + struct resource_entry *win; + int reg_no; + int err = 0; + int irq; + LIST_HEAD(res); + + if (!dev->of_node) + return -ENODEV; + + port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL); + if (!port) + return -ENOMEM; + + irq = platform_get_irq_byname(pdev, "pcie-sys"); + if (irq < 0) { + dev_err(dev, "missing pcie_sys IRQ resource\n"); + return -EINVAL; + } + err = devm_request_irq(dev, irq, rockchip_pcie_subsys_irq_handler, + IRQF_SHARED, "pcie-sys", port); + if (err) { + dev_err(dev, "failed to request pcie subsystem irq\n"); + return err; + } + + port->irq = platform_get_irq_byname(pdev, "pcie-legacy"); + if (port->irq < 0) { + dev_err(dev, "missing pcie_legacy IRQ resource\n"); + return -EINVAL; + } + err = devm_request_irq(dev, port->irq, + rockchip_pcie_legacy_int_handler, + IRQF_SHARED, + "pcie-legacy", + port); + if (err) { + dev_err(&pdev->dev, "failed to request pcie-legacy irq\n"); + return err; + } + + irq = platform_get_irq_byname(pdev, "pcie-client"); + if (irq < 0) { + dev_err(dev, "missing pcie-client IRQ resource\n"); + return -EINVAL; + } + err = devm_request_irq(dev, irq, rockchip_pcie_client_irq_handler, + IRQF_SHARED, "pcie-client", port); + if (err) { + dev_err(dev, "failed to request pcie client irq\n"); + return err; + } + + port->dev = dev; + err = rockchip_pcie_parse_dt(port); + if (err) { + dev_err(dev, "Parsing DT failed\n"); + return err; + } + + err = rockchip_pcie_init_port(port); + if (err) + return err; + + platform_set_drvdata(pdev, port); + + rockchip_pcie_enable_interrupts(port); + if (!IS_ENABLED(CONFIG_PCI_MSI)) { + err = rockchip_pcie_init_irq_domain(port); + if (err < 0) + return err; + } + + err = of_pci_get_host_bridge_resources(dev->of_node, 0, 0xff, + &res, &port->io_base); + if (err) + return err; + /* Get the I/O and memory ranges from DT */ + resource_list_for_each_entry(win, &res) { + switch (resource_type(win->res)) { + case IORESOURCE_IO: + port->io = win->res; + port->io->name = "I/O"; + port->io_size = resource_size(port->io); + port->io_bus_addr = port->io->start - win->offset; + err = pci_remap_iospace(port->io, port->io_base); + if (err) { + dev_warn(port->dev, "error %d: failed to map resource %pR\n", + err, port->io); + continue; + } + break; + case IORESOURCE_MEM: + port->mem = win->res; + port->mem->name = "MEM"; + port->mem_size = resource_size(port->mem); + port->mem_bus_addr = port->mem->start - win->offset; + break; + case 0: + port->cfg = win->res; + break; + case IORESOURCE_BUS: + port->busn = win->res; + break; + default: + continue; + } + } + + pcie_write(port, 0x6040000, PCIE_RC_CONFIG_BASE + 0x8); + pcie_write(port, 0x0, PCIE_CORE_CTRL_MGMT_BASE + 0x300); + + pcie_write(port, (RC_REGION_0_ADDR_TRANS_L + RC_REGION_0_PASS_BITS), + PCIE_CORE_AXI_CONF_BASE); + pcie_write(port, RC_REGION_0_ADDR_TRANS_H, + PCIE_CORE_AXI_CONF_BASE + 0x4); + pcie_write(port, 0x0080000a, PCIE_CORE_AXI_CONF_BASE + 0x8); + pcie_write(port, 0x00000000, PCIE_CORE_AXI_CONF_BASE + 0xc); + + for (reg_no = 0; reg_no < (port->mem_size >> 20); reg_no++) { + err = rockchip_pcie_prog_ob_atu(port, reg_no + 1, + AXI_WRAPPER_MEM_WRITE, + 20 - 1, + port->mem_bus_addr + + (reg_no << 20), + 0); + if (err) { + dev_err(dev, "Program RC outbound atu failed\n"); + return err; + } + } + + err = rockchip_pcie_prog_ib_atu(port, 2, 32 - 1, 0x0, 0); + if (err) { + dev_err(dev, "Program RC inbound atu failed\n"); + return err; + } + + rockchip_pcie_msi_enable(port); + + port->root_bus_nr = port->busn->start; + if (IS_ENABLED(CONFIG_PCI_MSI)) { + bus = pci_scan_root_bus_msi(port->dev, port->root_bus_nr, + &rockchip_pcie_ops, port, &res, + port->msi); + } else { + bus = pci_scan_root_bus(&pdev->dev, 0, + &rockchip_pcie_ops, port, &res); + } + if (!bus) + return -ENOMEM; + + if (!pci_has_flag(PCI_PROBE_ONLY)) { + pci_bus_size_bridges(bus); + pci_bus_assign_resources(bus); + list_for_each_entry(child, &bus->children, node) + pcie_bus_configure_settings(child); + } + + pci_bus_add_devices(bus); + + return err; +} + +static int rockchip_pcie_remove(struct platform_device *pdev) +{ + struct rockchip_pcie_port *port = platform_get_drvdata(pdev); + + clk_disable_unprepare(port->hclk_pcie); + clk_disable_unprepare(port->aclk_perf_pcie); + clk_disable_unprepare(port->aclk_pcie); + clk_disable_unprepare(port->clk_pciephy_ref); + + return 0; +} + +static const struct of_device_id rockchip_pcie_of_match[] = { + { .compatible = "rockchip,rk3399-pcie", }, + {} +}; + +static struct platform_driver rockchip_pcie_driver = { + .driver = { + .name = "rockchip-pcie", + .of_match_table = rockchip_pcie_of_match, + .suppress_bind_attrs = true, + }, + .probe = rockchip_pcie_probe, + .remove = rockchip_pcie_remove, +}; +module_platform_driver(rockchip_pcie_driver); + +MODULE_AUTHOR("Rockchip Inc"); +MODULE_DESCRIPTION("Rockchip AXI PCIe driver"); +MODULE_LICENSE("GPL v2");
RK3399 has a PCIe controller which can be used as Root Complex. This driver supports a PCIe controller as Root Complex mode. Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com> --- drivers/pci/host/Kconfig | 12 + drivers/pci/host/Makefile | 1 + drivers/pci/host/pcie-rockchip.c | 1181 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 1194 insertions(+) create mode 100644 drivers/pci/host/pcie-rockchip.c