Re: [PATCH 2/2] pci: Add PCIe driver for Rockchip Soc
From: Shawn Lin
Date: Sun May 22 2016 - 20:48:51 EST
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@xxxxxxxxxxxxxx>
---
[...]
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@xxxxxxxxxxxxxx>
+ * Wenrui Li <wenrui.li@xxxxxxxxxxxxxx>
+ * 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
--
Best Regards
Shawn Lin