[PATCH v6 0/2] add the Amlogic Meson PCIe controller driver

[PATCH v6 0/2] add the Amlogic Meson PCIe controller driver

[Hanjie Lin]

The Amlogic Meson PCIe host controller is based on the Synopsys DesignWare
PCI core. This patchset add the driver and dt-bindings of the controller.
Changes since v5: [4]
 - update MAINTAINER file in alphabetical order
 - remove meaningless comment
 - use ERR_PTR function instead of (void *) cast
 - use is_power_of_2(size) instead of size & (size - 1)
 - add comment for PCI_CLASS_REVISION register operation
 
Changes since v4: [3]
 - fix kbuild test robot and compile warnings

Changes since v3: [2]
 - modify subject format
 - update Kconfig
 - update MAINTAINER file
 - add comment and error handle for meson_pcie_get_mem_shared()
 - drop useless initialization code
 - add comment for meson_size_to_payload()
 - optimize meson_pcie_establish_link() return code
 - optimize meson_pcie_enable_interrupts() redundant function
 - drop device_attch related code
 - drop dw_pcie_ops read_dbi and write_dbi function
 - add error handle for meson_add_pcie_port() when probe

Changes since v2: [1]
 - abandon phy driver, move reset to the controller
 - use devm_add_action_or_reset() to use clock res
 - format correcting

Changes since v1: [0]
 - use gpio lib instead open code
 - move 'apb' and 'port' reset from phy driver
 - format correcting

[0] : https://lkml.kernel.org/r/1534227522-186798-1-git-send-email-hanjie.lin at amlogic.com
[1] : https://lkml.kernel.org/r/1535096165-45827-1-git-send-email-hanjie.lin at amlogic.com
[2] : https://lkml.kernel.org/r/1537509820-52040-1-git-send-email-hanjie.lin at amlogic.com 
[3] : https://lkml.kernel.org/r/1538999834-156423-3-git-send-email-hanjie.lin at amlogic.com
[4] : https://lkml.kernel.org/r/1539049990-30810-1-git-send-email-hanjie.lin at amlogic.com

Yue Wang (2):
  dt-bindings: PCI: meson: add DT bindings for Amlogic Meson PCIe
    controller
  PCI: amlogic: Add the Amlogic Meson PCIe controller driver

 .../devicetree/bindings/pci/amlogic,meson-pcie.txt |  70 +++
 MAINTAINERS                                        |   7 +
 drivers/pci/controller/dwc/Kconfig                 |  10 +
 drivers/pci/controller/dwc/Makefile                |   1 +
 drivers/pci/controller/dwc/pci-meson.c             | 597 +++++++++++++++++++++
 5 files changed, 685 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/amlogic,meson-pcie.txt
 create mode 100644 drivers/pci/controller/dwc/pci-meson.c

-- 
2.7.4

[PATCH v6 1/2] dt-bindings: PCI: meson: add DT bindings for Amlogic Meson PCIe controller

[Hanjie Lin]

From: Yue Wang <yue.wang@amlogic.com>

The Amlogic Meson PCIe host controller is based on the Synopsys DesignWare
PCI core. This patch adds documentation for the DT bindings in Meson PCIe
controller.

Signed-off-by: Yue Wang <yue.wang@amlogic.com>
Signed-off-by: Hanjie Lin <hanjie.lin@amlogic.com>
Reviewed-by: Rob Herring <robh@kernel.org>
---
 .../devicetree/bindings/pci/amlogic,meson-pcie.txt | 70 ++++++++++++++++++++++
 1 file changed, 70 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/amlogic,meson-pcie.txt

diff --git a/Documentation/devicetree/bindings/pci/amlogic,meson-pcie.txt b/Documentation/devicetree/bindings/pci/amlogic,meson-pcie.txt
new file mode 100644
index 0000000..12b18f8
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/amlogic,meson-pcie.txt
@@ -0,0 +1,70 @@
+Amlogic Meson AXG DWC PCIE SoC controller
+
+Amlogic Meson PCIe host controller is based on the Synopsys DesignWare PCI core.
+It shares common functions with the PCIe DesignWare core driver and
+inherits common properties defined in
+Documentation/devicetree/bindings/pci/designware-pci.txt.
+
+Additional properties are described here:
+
+Required properties:
+- compatible:
+	should contain "amlogic,axg-pcie" to identify the core.
+- reg:
+	should contain the configuration address space.
+- reg-names: Must be
+	- "elbi"	External local bus interface registers
+	- "cfg"		Meson specific registers
+	- "phy"		Meson PCIE PHY registers
+	- "config"	PCIe configuration space
+- reset-gpios: The GPIO to generate PCIe PERST# assert and deassert signal.
+- clocks: Must contain an entry for each entry in clock-names.
+- clock-names: Must include the following entries:
+	- "pclk"       PCIe GEN 100M PLL clock
+	- "port"       PCIe_x(A or B) RC clock gate
+	- "general"    PCIe Phy clock
+	- "mipi"       PCIe_x(A or B) 100M ref clock gate
+- resets: phandle to the reset lines.
+- reset-names: must contain "phy" "port" and "apb"
+       - "phy"         Share PHY reset
+       - "port"        Port A or B reset
+       - "apb"         Share APB reset
+- device_type:
+	should be "pci". As specified in designware-pcie.txt
+
+
+Example configuration:
+
+	pcie: pcie at f9800000 {
+			compatible = "amlogic,axg-pcie", "snps,dw-pcie";
+			reg = <0x0 0xf9800000 0x0 0x400000
+					0x0 0xff646000 0x0 0x2000
+					0x0 0xff644000 0x0 0x2000
+					0x0 0xf9f00000 0x0 0x100000>;
+			reg-names = "elbi", "cfg", "phy", "config";
+			reset-gpios = <&gpio GPIOX_19 GPIO_ACTIVE_HIGH>;
+			interrupts = <GIC_SPI 177 IRQ_TYPE_EDGE_RISING>;
+			#interrupt-cells = <1>;
+			interrupt-map-mask = <0 0 0 0>;
+			interrupt-map = <0 0 0 0 &gic GIC_SPI 179 IRQ_TYPE_EDGE_RISING>;
+			bus-range = <0x0 0xff>;
+			#address-cells = <3>;
+			#size-cells = <2>;
+			device_type = "pci";
+			ranges = <0x82000000 0 0 0x0 0xf9c00000 0 0x00300000>;
+
+			clocks = <&clkc CLKID_USB
+					&clkc CLKID_MIPI_ENABLE
+					&clkc CLKID_PCIE_A
+					&clkc CLKID_PCIE_CML_EN0>;
+			clock-names = "general",
+					"mipi",
+					"pclk",
+					"port";
+			resets = <&reset RESET_PCIE_PHY>,
+				<&reset RESET_PCIE_A>,
+				<&reset RESET_PCIE_APB>;
+			reset-names = "phy",
+					"port",
+					"apb";
+	};
-- 
2.7.4

[PATCH v6 2/2] PCI: amlogic: Add the Amlogic Meson PCIe controller driver

[Hanjie Lin]

From: Yue Wang <yue.wang@amlogic.com>

The Amlogic Meson PCIe host controller is based on the Synopsys DesignWare
PCI core. This patch adds the driver support for Meson PCIe controller.

Signed-off-by: Yue Wang <yue.wang@amlogic.com>
Signed-off-by: Hanjie Lin <hanjie.lin@amlogic.com>
---
 MAINTAINERS                            |   7 +
 drivers/pci/controller/dwc/Kconfig     |  10 +
 drivers/pci/controller/dwc/Makefile    |   1 +
 drivers/pci/controller/dwc/pci-meson.c | 597 +++++++++++++++++++++++++++++++++
 4 files changed, 615 insertions(+)
 create mode 100644 drivers/pci/controller/dwc/pci-meson.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 77b1174..4fb9098 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11530,6 +11530,13 @@ T:	git git://git.kernel.org/pub/scm/linux/kernel/git/lpieralisi/pci.git/
 S:	Supported
 F:	drivers/pci/controller/
 
+PCIE DRIVER FOR AMLOGIC MESON
+M:	Yue Wang <yue.wang@Amlogic.com>
+L:	linux-pci at vger.kernel.org
+L:	linux-amlogic at lists.infradead.org
+S:	Maintained
+F:	drivers/pci/controller/dwc/pci-meson.c
+
 PCIE DRIVER FOR AXIS ARTPEC
 M:	Jesper Nilsson <jesper.nilsson@axis.com>
 L:	linux-arm-kernel at axis.com
diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
index 91b0194..7800322 100644
--- a/drivers/pci/controller/dwc/Kconfig
+++ b/drivers/pci/controller/dwc/Kconfig
@@ -193,4 +193,14 @@ config PCIE_HISI_STB
 	help
           Say Y here if you want PCIe controller support on HiSilicon STB SoCs
 
+config PCI_MESON
+	bool "MESON PCIe controller"
+	depends on PCI_MSI_IRQ_DOMAIN
+	select PCIE_DW_HOST
+	help
+	  Say Y here if you want to enable PCI controller support on Amlogic
+	  SoCs. The PCI controller on Amlogic is based on DesignWare hardware
+	  and therefore the driver re-uses the DesignWare core functions to
+	  implement the driver.
+
 endmenu
diff --git a/drivers/pci/controller/dwc/Makefile b/drivers/pci/controller/dwc/Makefile
index fcf91ea..e05a015 100644
--- a/drivers/pci/controller/dwc/Makefile
+++ b/drivers/pci/controller/dwc/Makefile
@@ -14,6 +14,7 @@ obj-$(CONFIG_PCIE_ARMADA_8K) += pcie-armada8k.o
 obj-$(CONFIG_PCIE_ARTPEC6) += pcie-artpec6.o
 obj-$(CONFIG_PCIE_KIRIN) += pcie-kirin.o
 obj-$(CONFIG_PCIE_HISI_STB) += pcie-histb.o
+obj-$(CONFIG_PCI_MESON) += pci-meson.o
 
 # The following drivers are for devices that use the generic ACPI
 # pci_root.c driver but don't support standard ECAM config access.
diff --git a/drivers/pci/controller/dwc/pci-meson.c b/drivers/pci/controller/dwc/pci-meson.c
new file mode 100644
index 0000000..ef30a5a
--- /dev/null
+++ b/drivers/pci/controller/dwc/pci-meson.c
@@ -0,0 +1,597 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * PCIe host controller driver for Amlogic MESON SoCs
+ *
+ * Copyright (c) 2018 Amlogic, inc.
+ * Author: Yue Wang <yue.wang@amlogic.com>
+ */
+
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/of_device.h>
+#include <linux/of_gpio.h>
+#include <linux/pci.h>
+#include <linux/platform_device.h>
+#include <linux/resource.h>
+#include <linux/types.h>
+#include <linux/reset.h>
+
+#include "pcie-designware.h"
+
+#define to_meson_pcie(x) dev_get_drvdata((x)->dev)
+
+/* External local bus interface registers */
+#define PLR_OFFSET			0x700
+#define PCIE_PORT_LINK_CTRL_OFF		(PLR_OFFSET + 0x10)
+#define FAST_LINK_MODE			BIT(7)
+#define LINK_CAPABLE_MASK		GENMASK(21, 16)
+#define LINK_CAPABLE_X1			BIT(16)
+
+#define PCIE_GEN2_CTRL_OFF		(PLR_OFFSET + 0x10c)
+#define NUM_OF_LANES_MASK		GENMASK(12, 8)
+#define NUM_OF_LANES_X1			BIT(8)
+#define DIRECT_SPEED_CHANGE		BIT(17)
+
+#define TYPE1_HDR_OFFSET		0x0
+#define PCIE_STATUS_COMMAND		(TYPE1_HDR_OFFSET + 0x04)
+#define PCI_IO_EN			BIT(0)
+#define PCI_MEM_SPACE_EN		BIT(1)
+#define PCI_BUS_MASTER_EN		BIT(2)
+
+#define PCIE_BASE_ADDR0			(TYPE1_HDR_OFFSET + 0x10)
+#define PCIE_BASE_ADDR1			(TYPE1_HDR_OFFSET + 0x14)
+
+#define PCIE_CAP_OFFSET			0x70
+#define PCIE_DEV_CTRL_DEV_STUS		(PCIE_CAP_OFFSET + 0x08)
+#define PCIE_CAP_MAX_PAYLOAD_MASK	GENMASK(7, 5)
+#define PCIE_CAP_MAX_PAYLOAD_SIZE(x)	((x) << 5)
+#define PCIE_CAP_MAX_READ_REQ_MASK	GENMASK(14, 12)
+#define PCIE_CAP_MAX_READ_REQ_SIZE(x)	((x) << 12)
+
+#define PCI_CLASS_REVISION_MASK		GENMASK(7, 0)
+
+/* PCIe specific config registers */
+#define PCIE_CFG0			0x0
+#define APP_LTSSM_ENABLE		BIT(7)
+
+#define PCIE_CFG_STATUS12		0x30
+#define IS_SMLH_LINK_UP(x)		((x) & (1 << 6))
+#define IS_RDLH_LINK_UP(x)		((x) & (1 << 16))
+#define IS_LTSSM_UP(x)			((((x) >> 10) & 0x1f) == 0x11)
+
+#define PCIE_CFG_STATUS17		0x44
+#define PM_CURRENT_STATE(x)		(((x) >> 7) & 0x1)
+
+#define WAIT_LINKUP_TIMEOUT		2000
+#define PORT_CLK_RATE			100000000UL
+#define MAX_PAYLOAD_SIZE		256
+#define MAX_READ_REQ_SIZE		256
+#define MESON_PCIE_PHY_POWERUP		0x1c
+#define PCIE_RESET_DELAY		500
+#define PCIE_SHARED_RESET		1
+#define PCIE_NORMAL_RESET		0
+
+enum pcie_data_rate {
+	PCIE_GEN1,
+	PCIE_GEN2,
+	PCIE_GEN3,
+	PCIE_GEN4
+};
+
+struct meson_pcie_mem_res {
+	void __iomem *elbi_base;
+	void __iomem *cfg_base;
+	void __iomem *phy_base;
+};
+
+struct meson_pcie_clk_res {
+	struct clk *clk;
+	struct clk *mipi_gate;
+	struct clk *port_clk;
+	struct clk *general_clk;
+};
+
+struct meson_pcie_rc_reset {
+	struct reset_control *phy;
+	struct reset_control *port;
+	struct reset_control *apb;
+};
+
+struct meson_pcie {
+	struct dw_pcie pci;
+	struct meson_pcie_mem_res mem_res;
+	struct meson_pcie_clk_res clk_res;
+	struct meson_pcie_rc_reset mrst;
+	struct gpio_desc *reset_gpio;
+
+	enum of_gpio_flags gpio_flag;
+	int pcie_num;
+	u32 port_num;
+};
+
+static struct reset_control *meson_pcie_get_reset(struct meson_pcie *mp,
+						  const char *id,
+						  u32 reset_type)
+{
+	struct device *dev = mp->pci.dev;
+	struct reset_control *reset;
+
+	if (reset_type == PCIE_SHARED_RESET)
+		reset = devm_reset_control_get_shared(dev, id);
+	else
+		reset = devm_reset_control_get(dev, id);
+
+	return reset;
+}
+
+static int meson_pcie_get_resets(struct meson_pcie *mp)
+{
+	struct meson_pcie_rc_reset *mrst = &mp->mrst;
+
+	mrst->phy = meson_pcie_get_reset(mp, "phy", PCIE_SHARED_RESET);
+	if (IS_ERR(mrst->phy))
+		return PTR_ERR(mrst->phy);
+	reset_control_deassert(mrst->phy);
+
+	mrst->port = meson_pcie_get_reset(mp, "port", PCIE_NORMAL_RESET);
+	if (IS_ERR(mrst->port))
+		return PTR_ERR(mrst->port);
+	reset_control_deassert(mrst->port);
+
+	mrst->apb = meson_pcie_get_reset(mp, "apb", PCIE_SHARED_RESET);
+	if (IS_ERR(mrst->apb))
+		return PTR_ERR(mrst->apb);
+	reset_control_deassert(mrst->apb);
+
+	return 0;
+}
+
+static void __iomem *meson_pcie_get_mem(struct platform_device *pdev,
+					struct meson_pcie *mp,
+					const char *id)
+{
+	struct device *dev = mp->pci.dev;
+	struct resource *res;
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, id);
+
+	return devm_ioremap_resource(dev, res);
+}
+
+static void __iomem *meson_pcie_get_mem_shared(struct platform_device *pdev,
+					       struct meson_pcie *mp,
+					       const char *id)
+{
+	struct device *dev = mp->pci.dev;
+	struct resource *res;
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, id);
+	if (!res) {
+		dev_err(dev, "No REG resource %s\n", id);
+		return ERR_PTR(ENXIO);
+	}
+
+	return devm_ioremap(dev, res->start, resource_size(res));
+}
+
+static int meson_pcie_get_mems(struct platform_device *pdev,
+			       struct meson_pcie *mp)
+{
+	mp->mem_res.elbi_base = meson_pcie_get_mem(pdev, mp, "elbi");
+	if (IS_ERR(mp->mem_res.elbi_base))
+		return PTR_ERR(mp->mem_res.elbi_base);
+
+	mp->mem_res.cfg_base = meson_pcie_get_mem(pdev, mp, "cfg");
+	if (IS_ERR(mp->mem_res.cfg_base))
+		return PTR_ERR(mp->mem_res.cfg_base);
+
+	/* Meson SoC has two PCI controllers use same phy register*/
+	mp->mem_res.phy_base = meson_pcie_get_mem_shared(pdev, mp, "phy");
+	if (IS_ERR(mp->mem_res.phy_base))
+		return PTR_ERR(mp->mem_res.phy_base);
+
+	return 0;
+}
+
+static void meson_pcie_power_on(struct meson_pcie *mp)
+{
+	writel(MESON_PCIE_PHY_POWERUP, mp->mem_res.phy_base);
+}
+
+static void meson_pcie_reset(struct meson_pcie *mp)
+{
+	struct meson_pcie_rc_reset *mrst = &mp->mrst;
+
+	reset_control_assert(mrst->phy);
+	udelay(PCIE_RESET_DELAY);
+	reset_control_deassert(mrst->phy);
+	udelay(PCIE_RESET_DELAY);
+
+	reset_control_assert(mrst->port);
+	reset_control_assert(mrst->apb);
+	udelay(PCIE_RESET_DELAY);
+	reset_control_deassert(mrst->port);
+	reset_control_deassert(mrst->apb);
+	udelay(PCIE_RESET_DELAY);
+}
+
+static inline struct clk *meson_pcie_probe_clock(struct device *dev,
+						 const char *id, u64 rate)
+{
+	struct clk *clk;
+	int ret;
+
+	clk = devm_clk_get(dev, id);
+	if (IS_ERR(clk))
+		return clk;
+
+	if (rate) {
+		ret = clk_set_rate(clk, rate);
+		if (ret) {
+			dev_err(dev, "set clk rate failed, ret = %d\n", ret);
+			return ERR_PTR(ret);
+		}
+	}
+
+	ret = clk_prepare_enable(clk);
+	if (ret) {
+		dev_err(dev, "couldn't enable clk\n");
+		return ERR_PTR(ret);
+	}
+
+	devm_add_action_or_reset(dev,
+				 (void (*) (void *))clk_disable_unprepare,
+				 clk);
+
+	return clk;
+}
+
+static int meson_pcie_probe_clocks(struct meson_pcie *mp)
+{
+	struct device *dev = mp->pci.dev;
+	struct meson_pcie_clk_res *res = &mp->clk_res;
+
+	res->port_clk = meson_pcie_probe_clock(dev, "port", PORT_CLK_RATE);
+	if (IS_ERR(res->port_clk))
+		return PTR_ERR(res->port_clk);
+
+	res->mipi_gate = meson_pcie_probe_clock(dev, "pcie_mipi_en", 0);
+	if (IS_ERR(res->mipi_gate))
+		return PTR_ERR(res->mipi_gate);
+
+	res->general_clk = meson_pcie_probe_clock(dev, "pcie_general", 0);
+	if (IS_ERR(res->general_clk))
+		return PTR_ERR(res->general_clk);
+
+	res->clk = meson_pcie_probe_clock(dev, "pcie", 0);
+	if (IS_ERR(res->clk))
+		return PTR_ERR(res->clk);
+
+	return 0;
+}
+
+static inline void meson_elb_writel(struct meson_pcie *mp, u32 val, u32 reg)
+{
+	writel(val, mp->mem_res.elbi_base + reg);
+}
+
+static inline u32 meson_elb_readl(struct meson_pcie *mp, u32 reg)
+{
+	return readl(mp->mem_res.elbi_base + reg);
+}
+
+static inline u32 meson_cfg_readl(struct meson_pcie *mp, u32 reg)
+{
+	return readl(mp->mem_res.cfg_base + reg);
+}
+
+static inline void meson_cfg_writel(struct meson_pcie *mp, u32 val, u32 reg)
+{
+	writel(val, mp->mem_res.cfg_base + reg);
+}
+
+static void meson_pcie_assert_reset(struct meson_pcie *mp)
+{
+	gpiod_set_value_cansleep(mp->reset_gpio, 0);
+	udelay(500);
+	gpiod_set_value_cansleep(mp->reset_gpio, 1);
+}
+
+static void meson_pcie_init_dw(struct meson_pcie *mp)
+{
+	u32 val;
+
+	val = meson_cfg_readl(mp, PCIE_CFG0);
+	val |= APP_LTSSM_ENABLE;
+	meson_cfg_writel(mp, val, PCIE_CFG0);
+
+	val = meson_elb_readl(mp, PCIE_PORT_LINK_CTRL_OFF);
+	val &= ~LINK_CAPABLE_MASK;
+	meson_elb_writel(mp, val, PCIE_PORT_LINK_CTRL_OFF);
+
+	val = meson_elb_readl(mp, PCIE_PORT_LINK_CTRL_OFF);
+	val |= LINK_CAPABLE_X1 | FAST_LINK_MODE;
+	meson_elb_writel(mp, val, PCIE_PORT_LINK_CTRL_OFF);
+
+	val = meson_elb_readl(mp, PCIE_GEN2_CTRL_OFF);
+	val &= ~NUM_OF_LANES_MASK;
+	meson_elb_writel(mp, val, PCIE_GEN2_CTRL_OFF);
+
+	val = meson_elb_readl(mp, PCIE_GEN2_CTRL_OFF);
+	val |= NUM_OF_LANES_X1 | DIRECT_SPEED_CHANGE;
+	meson_elb_writel(mp, val, PCIE_GEN2_CTRL_OFF);
+
+	meson_elb_writel(mp, 0x0, PCIE_BASE_ADDR0);
+	meson_elb_writel(mp, 0x0, PCIE_BASE_ADDR1);
+}
+
+static int meson_size_to_payload(struct meson_pcie *mp, int size)
+{
+	struct device *dev = mp->pci.dev;
+
+	/*
+	 * dwc supports 2^(val+7) payload size, which val is 0~5 default to 1.
+	 * So if input size is not 2^order alignment or less than 2^7 or bigger
+	 * than 2^12, just set to default size 2^(1+7).
+	 */
+	if (!is_power_of_2(size) || size < 128 || size > 4096) {
+		dev_warn(dev, "playload size %d, set to default 256\n", size);
+		return 1;
+	}
+
+	return fls(size) - 8;
+}
+
+static void meson_set_max_payload(struct meson_pcie *mp, int size)
+{
+	u32 val = 0;
+	int max_payload_size = meson_size_to_payload(mp, size);
+
+	val = meson_elb_readl(mp, PCIE_DEV_CTRL_DEV_STUS);
+	val &= ~PCIE_CAP_MAX_PAYLOAD_MASK;
+	meson_elb_writel(mp, val, PCIE_DEV_CTRL_DEV_STUS);
+
+	val = meson_elb_readl(mp, PCIE_DEV_CTRL_DEV_STUS);
+	val |= PCIE_CAP_MAX_PAYLOAD_SIZE(max_payload_size);
+	meson_elb_writel(mp, val, PCIE_DEV_CTRL_DEV_STUS);
+}
+
+static void meson_set_max_rd_req_size(struct meson_pcie *mp, int size)
+{
+	u32 val;
+	int max_rd_req_size = meson_size_to_payload(mp, size);
+
+	val = meson_elb_readl(mp, PCIE_DEV_CTRL_DEV_STUS);
+	val &= ~PCIE_CAP_MAX_READ_REQ_MASK;
+	meson_elb_writel(mp, val, PCIE_DEV_CTRL_DEV_STUS);
+
+	val = meson_elb_readl(mp, PCIE_DEV_CTRL_DEV_STUS);
+	val |= PCIE_CAP_MAX_READ_REQ_SIZE(max_rd_req_size);
+	meson_elb_writel(mp, val, PCIE_DEV_CTRL_DEV_STUS);
+}
+
+static inline void meson_enable_memory_space(struct meson_pcie *mp)
+{
+	/* Set the RC Bus Master, Memory Space and I/O Space enables */
+	meson_elb_writel(mp, PCI_IO_EN | PCI_MEM_SPACE_EN | PCI_BUS_MASTER_EN,
+			 PCIE_STATUS_COMMAND);
+}
+
+static int meson_pcie_establish_link(struct meson_pcie *mp)
+{
+	struct dw_pcie *pci = &mp->pci;
+	struct pcie_port *pp = &pci->pp;
+
+	meson_pcie_init_dw(mp);
+	meson_set_max_payload(mp, MAX_PAYLOAD_SIZE);
+	meson_set_max_rd_req_size(mp, MAX_READ_REQ_SIZE);
+
+	dw_pcie_setup_rc(pp);
+	meson_enable_memory_space(mp);
+
+	meson_pcie_assert_reset(mp);
+
+	return dw_pcie_wait_for_link(pci);
+}
+
+static void meson_pcie_enable_interrupts(struct meson_pcie *mp)
+{
+	if (IS_ENABLED(CONFIG_PCI_MSI))
+		dw_pcie_msi_init(&mp->pci.pp);
+}
+
+static int meson_pcie_rd_own_conf(struct pcie_port *pp, int where, int size,
+				  u32 *val)
+{
+	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
+
+	/*
+	 * there is a bug of MESON AXG pcie controller that software can not
+	 * programe PCI_CLASS_DEVICE register, so we must return a fake right
+	 * value to ensure driver could probe successfully.
+	 */
+	if (where == PCI_CLASS_REVISION) {
+		*val = readl(pci->dbi_base + PCI_CLASS_REVISION);
+		/* keep revision id */
+		*val &= PCI_CLASS_REVISION_MASK;
+		*val |= PCI_CLASS_BRIDGE_PCI << 16;
+		return PCIBIOS_SUCCESSFUL;
+	}
+
+	return dw_pcie_read(pci->dbi_base + where, size, val);
+}
+
+static int meson_pcie_wr_own_conf(struct pcie_port *pp, int where,
+				  int size, u32 val)
+{
+	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
+
+	return dw_pcie_write(pci->dbi_base + where, size, val);
+}
+
+static int meson_pcie_link_up(struct dw_pcie *pci)
+{
+	struct meson_pcie *mp = to_meson_pcie(pci);
+	struct device *dev = pci->dev;
+	u32 smlh_up = 0;
+	u32 ltssm_up = 0;
+	u32 rdlh_up = 0;
+	u32 speed_okay = 0;
+	u32 cnt = 0;
+	u32 state12, state17;
+
+	while (smlh_up == 0 || rdlh_up == 0 || ltssm_up == 0 ||
+	       speed_okay == 0) {
+		udelay(20);
+
+		state12 = meson_cfg_readl(mp, PCIE_CFG_STATUS12);
+		state17 = meson_cfg_readl(mp, PCIE_CFG_STATUS17);
+		smlh_up = IS_SMLH_LINK_UP(state12);
+		rdlh_up = IS_RDLH_LINK_UP(state12);
+		ltssm_up = IS_LTSSM_UP(state12);
+
+		if (PM_CURRENT_STATE(state17) < PCIE_GEN3)
+			speed_okay = 1;
+
+		if (smlh_up)
+			dev_dbg(dev, "smlh_link_up is on\n");
+		if (rdlh_up)
+			dev_dbg(dev, "rdlh_link_up is on\n");
+		if (ltssm_up)
+			dev_dbg(dev, "ltssm_up is on\n");
+		if (speed_okay)
+			dev_dbg(dev, "speed_okay\n");
+
+		cnt++;
+
+		if (cnt >= WAIT_LINKUP_TIMEOUT) {
+			dev_err(dev, "Error: Wait linkup timeout.\n");
+			return 0;
+		}
+	}
+
+	return 1;
+}
+
+static int meson_pcie_host_init(struct pcie_port *pp)
+{
+	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
+	struct meson_pcie *mp = to_meson_pcie(pci);
+	int ret;
+
+	ret = meson_pcie_establish_link(mp);
+	if (ret)
+		return ret;
+
+	meson_pcie_enable_interrupts(mp);
+
+	return 0;
+}
+
+static const struct dw_pcie_host_ops meson_pcie_host_ops = {
+	.rd_own_conf = meson_pcie_rd_own_conf,
+	.wr_own_conf = meson_pcie_wr_own_conf,
+	.host_init = meson_pcie_host_init,
+};
+
+static int meson_add_pcie_port(struct meson_pcie *mp,
+			       struct platform_device *pdev)
+{
+	struct dw_pcie *pci = &mp->pci;
+	struct pcie_port *pp = &pci->pp;
+	struct device *dev = &pdev->dev;
+	int ret;
+
+	if (IS_ENABLED(CONFIG_PCI_MSI)) {
+		pp->msi_irq = platform_get_irq(pdev, 0);
+		if (pp->msi_irq < 0) {
+			dev_err(dev, "failed to get msi irq\n");
+			return pp->msi_irq;
+		}
+	}
+
+	pp->ops = &meson_pcie_host_ops;
+	pci->dbi_base = mp->mem_res.elbi_base;
+
+	ret = dw_pcie_host_init(pp);
+	if (ret) {
+		dev_err(dev, "failed to initialize host\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+static const struct dw_pcie_ops dw_pcie_ops = {
+	.link_up = meson_pcie_link_up,
+};
+
+static int meson_pcie_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct dw_pcie *pci;
+	struct meson_pcie *mp;
+	int ret;
+
+	mp = devm_kzalloc(dev, sizeof(*mp), GFP_KERNEL);
+	if (!mp)
+		return -ENOMEM;
+
+	pci = &mp->pci;
+	pci->dev = dev;
+	pci->ops = &dw_pcie_ops;
+
+	mp->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
+	if (IS_ERR(mp->reset_gpio)) {
+		dev_err(dev, "Get reset gpio failed\n");
+		return PTR_ERR(mp->reset_gpio);
+	}
+
+	ret = meson_pcie_get_resets(mp);
+	if (ret) {
+		dev_err(dev, "Get reset resource failed, %d\n", ret);
+		return ret;
+	}
+
+	ret = meson_pcie_get_mems(pdev, mp);
+	if (ret) {
+		dev_err(dev, "Get memory resource failed, %d\n", ret);
+		return ret;
+	}
+
+	meson_pcie_power_on(mp);
+	meson_pcie_reset(mp);
+
+	ret = meson_pcie_probe_clocks(mp);
+	if (ret) {
+		dev_err(dev, "Init clock resources failed, %d\n", ret);
+		return ret;
+	}
+
+	platform_set_drvdata(pdev, mp);
+
+	ret = meson_add_pcie_port(mp, pdev);
+	if (ret < 0) {
+		dev_err(dev, "Add PCIE port failed, %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static const struct of_device_id meson_pcie_of_match[] = {
+	{
+		.compatible = "amlogic,axg-pcie",
+	},
+	{},
+};
+
+static struct platform_driver meson_pcie_driver = {
+	.probe = meson_pcie_probe,
+	.driver = {
+		.name = "meson-pcie",
+		.of_match_table = meson_pcie_of_match,
+	},
+};
+
+builtin_platform_driver(meson_pcie_driver);
-- 
2.7.4

[PATCH v6 2/2] PCI: amlogic: Add the Amlogic Meson PCIe controller driver

[Dan Carpenter]

Hi Yue,

url:    https://github.com/0day-ci/linux/commits/Hanjie-Lin/dt-bindings-PCI-meson-add-DT-bindings-for-Amlogic-Meson-PCIe-controller/20181122-225955
base:   https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next

smatch warnings:
drivers/pci/controller/dwc/pci-meson.c:171 meson_pcie_get_mem_shared() error: passing non negative 6 to ERR_PTR

# https://github.com/0day-ci/linux/commit/c882cdc75e49b6de65cd3d95ebf688272af6b5f9
git remote add linux-review https://github.com/0day-ci/linux
git remote update linux-review
git checkout c882cdc75e49b6de65cd3d95ebf688272af6b5f9
vim +171 drivers/pci/controller/dwc/pci-meson.c

c882cdc75 Yue Wang 2018-11-22  160  
c882cdc75 Yue Wang 2018-11-22  161  static void __iomem *meson_pcie_get_mem_shared(struct platform_device *pdev,
c882cdc75 Yue Wang 2018-11-22  162  					       struct meson_pcie *mp,
c882cdc75 Yue Wang 2018-11-22  163  					       const char *id)
c882cdc75 Yue Wang 2018-11-22  164  {
c882cdc75 Yue Wang 2018-11-22  165  	struct device *dev = mp->pci.dev;
c882cdc75 Yue Wang 2018-11-22  166  	struct resource *res;
c882cdc75 Yue Wang 2018-11-22  167  
c882cdc75 Yue Wang 2018-11-22  168  	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, id);
c882cdc75 Yue Wang 2018-11-22  169  	if (!res) {
c882cdc75 Yue Wang 2018-11-22  170  		dev_err(dev, "No REG resource %s\n", id);
c882cdc75 Yue Wang 2018-11-22 @171  		return ERR_PTR(ENXIO);
                                                               ^^^^^
                                                               -ENXIO

c882cdc75 Yue Wang 2018-11-22  172  	}
c882cdc75 Yue Wang 2018-11-22  173  
c882cdc75 Yue Wang 2018-11-22  174  	return devm_ioremap(dev, res->start, resource_size(res));
c882cdc75 Yue Wang 2018-11-22  175  }
c882cdc75 Yue Wang 2018-11-22  176  

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

Re: [PATCH v6 2/2] PCI: amlogic: Add the Amlogic Meson PCIe controller driver

[Lorenzo Pieralisi]

On Thu, Nov 22, 2018 at 04:53:54PM +0800, Hanjie Lin wrote:

[...]

> +static int meson_pcie_rd_own_conf(struct pcie_port *pp, int where, int size,
> +				  u32 *val)
> +{
> +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> +
> +	/*
> +	 * there is a bug of MESON AXG pcie controller that software can not
> +	 * programe PCI_CLASS_DEVICE register, so we must return a fake right
> +	 * value to ensure driver could probe successfully.
> +	 */
> +	if (where == PCI_CLASS_REVISION) {
> +		*val = readl(pci->dbi_base + PCI_CLASS_REVISION);
> +		/* keep revision id */
> +		*val &= PCI_CLASS_REVISION_MASK;
> +		*val |= PCI_CLASS_BRIDGE_PCI << 16;
> +		return PCIBIOS_SUCCESSFUL;
> +	}

As I said before, this looks broken. If this code (or other drivers with
the same broken assumptions, eg dwc/pcie-qcom.c) carries out a, say,
byte sized config access of eg PCI_CLASS_DEVICE you will get junk out of
it according to your comment above.

I would like to pick Bjorn's brain on this to see what we can really do
to fix this (and other) drivers.

Thanks,
Lorenzo

> +	return dw_pcie_read(pci->dbi_base + where, size, val);
> +}
> +
> +static int meson_pcie_wr_own_conf(struct pcie_port *pp, int where,
> +				  int size, u32 val)
> +{
> +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> +
> +	return dw_pcie_write(pci->dbi_base + where, size, val);
> +}
> +
> +static int meson_pcie_link_up(struct dw_pcie *pci)
> +{
> +	struct meson_pcie *mp = to_meson_pcie(pci);
> +	struct device *dev = pci->dev;
> +	u32 smlh_up = 0;
> +	u32 ltssm_up = 0;
> +	u32 rdlh_up = 0;
> +	u32 speed_okay = 0;
> +	u32 cnt = 0;
> +	u32 state12, state17;
> +
> +	while (smlh_up == 0 || rdlh_up == 0 || ltssm_up == 0 ||
> +	       speed_okay == 0) {
> +		udelay(20);
> +
> +		state12 = meson_cfg_readl(mp, PCIE_CFG_STATUS12);
> +		state17 = meson_cfg_readl(mp, PCIE_CFG_STATUS17);
> +		smlh_up = IS_SMLH_LINK_UP(state12);
> +		rdlh_up = IS_RDLH_LINK_UP(state12);
> +		ltssm_up = IS_LTSSM_UP(state12);
> +
> +		if (PM_CURRENT_STATE(state17) < PCIE_GEN3)
> +			speed_okay = 1;
> +
> +		if (smlh_up)
> +			dev_dbg(dev, "smlh_link_up is on\n");
> +		if (rdlh_up)
> +			dev_dbg(dev, "rdlh_link_up is on\n");
> +		if (ltssm_up)
> +			dev_dbg(dev, "ltssm_up is on\n");
> +		if (speed_okay)
> +			dev_dbg(dev, "speed_okay\n");
> +
> +		cnt++;
> +
> +		if (cnt >= WAIT_LINKUP_TIMEOUT) {
> +			dev_err(dev, "Error: Wait linkup timeout.\n");
> +			return 0;
> +		}
> +	}
> +
> +	return 1;
> +}
> +
> +static int meson_pcie_host_init(struct pcie_port *pp)
> +{
> +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> +	struct meson_pcie *mp = to_meson_pcie(pci);
> +	int ret;
> +
> +	ret = meson_pcie_establish_link(mp);
> +	if (ret)
> +		return ret;
> +
> +	meson_pcie_enable_interrupts(mp);
> +
> +	return 0;
> +}
> +
> +static const struct dw_pcie_host_ops meson_pcie_host_ops = {
> +	.rd_own_conf = meson_pcie_rd_own_conf,
> +	.wr_own_conf = meson_pcie_wr_own_conf,
> +	.host_init = meson_pcie_host_init,
> +};
> +
> +static int meson_add_pcie_port(struct meson_pcie *mp,
> +			       struct platform_device *pdev)
> +{
> +	struct dw_pcie *pci = &mp->pci;
> +	struct pcie_port *pp = &pci->pp;
> +	struct device *dev = &pdev->dev;
> +	int ret;
> +
> +	if (IS_ENABLED(CONFIG_PCI_MSI)) {
> +		pp->msi_irq = platform_get_irq(pdev, 0);
> +		if (pp->msi_irq < 0) {
> +			dev_err(dev, "failed to get msi irq\n");
> +			return pp->msi_irq;
> +		}
> +	}
> +
> +	pp->ops = &meson_pcie_host_ops;
> +	pci->dbi_base = mp->mem_res.elbi_base;
> +
> +	ret = dw_pcie_host_init(pp);
> +	if (ret) {
> +		dev_err(dev, "failed to initialize host\n");
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct dw_pcie_ops dw_pcie_ops = {
> +	.link_up = meson_pcie_link_up,
> +};
> +
> +static int meson_pcie_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct dw_pcie *pci;
> +	struct meson_pcie *mp;
> +	int ret;
> +
> +	mp = devm_kzalloc(dev, sizeof(*mp), GFP_KERNEL);
> +	if (!mp)
> +		return -ENOMEM;
> +
> +	pci = &mp->pci;
> +	pci->dev = dev;
> +	pci->ops = &dw_pcie_ops;
> +
> +	mp->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
> +	if (IS_ERR(mp->reset_gpio)) {
> +		dev_err(dev, "Get reset gpio failed\n");
> +		return PTR_ERR(mp->reset_gpio);
> +	}
> +
> +	ret = meson_pcie_get_resets(mp);
> +	if (ret) {
> +		dev_err(dev, "Get reset resource failed, %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = meson_pcie_get_mems(pdev, mp);
> +	if (ret) {
> +		dev_err(dev, "Get memory resource failed, %d\n", ret);
> +		return ret;
> +	}
> +
> +	meson_pcie_power_on(mp);
> +	meson_pcie_reset(mp);
> +
> +	ret = meson_pcie_probe_clocks(mp);
> +	if (ret) {
> +		dev_err(dev, "Init clock resources failed, %d\n", ret);
> +		return ret;
> +	}
> +
> +	platform_set_drvdata(pdev, mp);
> +
> +	ret = meson_add_pcie_port(mp, pdev);
> +	if (ret < 0) {
> +		dev_err(dev, "Add PCIE port failed, %d\n", ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id meson_pcie_of_match[] = {
> +	{
> +		.compatible = "amlogic,axg-pcie",
> +	},
> +	{},
> +};
> +
> +static struct platform_driver meson_pcie_driver = {
> +	.probe = meson_pcie_probe,
> +	.driver = {
> +		.name = "meson-pcie",
> +		.of_match_table = meson_pcie_of_match,
> +	},
> +};
> +
> +builtin_platform_driver(meson_pcie_driver);
> -- 
> 2.7.4
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v6 2/2] PCI: amlogic: Add the Amlogic Meson PCIe controller driver

[Bjorn Helgaas]

On Mon, Dec 03, 2018 at 04:41:50PM +0000, Lorenzo Pieralisi wrote:
> On Thu, Nov 22, 2018 at 04:53:54PM +0800, Hanjie Lin wrote:
> 
> [...]
> 
> > +static int meson_pcie_rd_own_conf(struct pcie_port *pp, int where, int size,
> > +				  u32 *val)
> > +{
> > +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > +
> > +	/*
> > +	 * there is a bug of MESON AXG pcie controller that software can not
> > +	 * programe PCI_CLASS_DEVICE register, so we must return a fake right
> > +	 * value to ensure driver could probe successfully.
> > +	 */
> > +	if (where == PCI_CLASS_REVISION) {
> > +		*val = readl(pci->dbi_base + PCI_CLASS_REVISION);
> > +		/* keep revision id */
> > +		*val &= PCI_CLASS_REVISION_MASK;
> > +		*val |= PCI_CLASS_BRIDGE_PCI << 16;
> > +		return PCIBIOS_SUCCESSFUL;
> > +	}
> 
> As I said before, this looks broken. If this code (or other drivers with
> the same broken assumptions, eg dwc/pcie-qcom.c) carries out a, say,
> byte sized config access of eg PCI_CLASS_DEVICE you will get junk out of
> it according to your comment above.
> 
> I would like to pick Bjorn's brain on this to see what we can really do
> to fix this (and other) drivers.

  - Check to see whether you're reading anything in the 32-bit dword at
    offset 0x08.

  - Do the 32-bit readl().

  - Insert the correct Sub-Class and Base Class code (you also throw
    away the Programming Interface; not sure why that is)

  - If you're reading something smaller than 32 bits, mask & shift as
    needed.  pci_bridge_emul_conf_read() does something similar that
    you might be able to copy.

Out of curiosity, what code depends on PCI_CLASS_BRIDGE_PCI?  There
are several places in the kernel that currently depend on it, but I
think several of them *should* be checking dev->hdr_type to identify a
type 1 header instead.

Bjorn

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v6 2/2] PCI: amlogic: Add the Amlogic Meson PCIe controller driver

[Hanjie Lin]

On 2018/12/4 6:57, Bjorn Helgaas wrote:
> On Mon, Dec 03, 2018 at 04:41:50PM +0000, Lorenzo Pieralisi wrote:
>> On Thu, Nov 22, 2018 at 04:53:54PM +0800, Hanjie Lin wrote:
>>
>> [...]
>>
>>> +static int meson_pcie_rd_own_conf(struct pcie_port *pp, int where, int size,
>>> +				  u32 *val)
>>> +{
>>> +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>>> +
>>> +	/*
>>> +	 * there is a bug of MESON AXG pcie controller that software can not
>>> +	 * programe PCI_CLASS_DEVICE register, so we must return a fake right
>>> +	 * value to ensure driver could probe successfully.
>>> +	 */
>>> +	if (where == PCI_CLASS_REVISION) {
>>> +		*val = readl(pci->dbi_base + PCI_CLASS_REVISION);
>>> +		/* keep revision id */
>>> +		*val &= PCI_CLASS_REVISION_MASK;
>>> +		*val |= PCI_CLASS_BRIDGE_PCI << 16;
>>> +		return PCIBIOS_SUCCESSFUL;
>>> +	}
>>
>> As I said before, this looks broken. If this code (or other drivers with
>> the same broken assumptions, eg dwc/pcie-qcom.c) carries out a, say,
>> byte sized config access of eg PCI_CLASS_DEVICE you will get junk out of
>> it according to your comment above.
>>
>> I would like to pick Bjorn's brain on this to see what we can really do
>> to fix this (and other) drivers.
> 
>   - Check to see whether you're reading anything in the 32-bit dword at
>     offset 0x08.
> 
>   - Do the 32-bit readl().
> 
>   - Insert the correct Sub-Class and Base Class code (you also throw
>     away the Programming Interface; not sure why that is)
> 
>   - If you're reading something smaller than 32 bits, mask & shift as
>     needed.  pci_bridge_emul_conf_read() does something similar that
>     you might be able to copy.
> 
> Out of curiosity, what code depends on PCI_CLASS_BRIDGE_PCI?  There
> are several places in the kernel that currently depend on it, but I
> think several of them *should* be checking dev->hdr_type to identify a
> type 1 header instead.
> 
> Bjorn
> 
> .
> 

Yes, it would be broken in particular scenes(eg: read 1 or 2 bytes from 0xa/PCI_CLASS_DEVICE)
that I didn't considered.
 
As your suggestion, I consider some code below may help this issue:
1, First call dw_pcie_read() help to read 1/2/4 bytes from register,
   request all other *size* bytes will return error and dw_pcie_read()
   will also check register alignment.
 
2, If dw_pcie_read() return success and *where* is 0x8/PCI_CLASS_DEVICE or 0xa/PCI_CLASS_REVISION,
   we may need to correct class code.
   As PCI_CLASS_REVISION is two-bytes register, so only when read 4 bytes from 0x8/PCI_CLASS_DEVICE
   or read 2 bytes from 0xa/PCI_CLASS_REVISION we should correct the class code.
 
ps: read 1 byte from 0xa/PCI_CLASS_REVISION or 0xb will get incorrect value.

static int meson_pcie_rd_own_conf(struct pcie_port *pp, int where, int size,
                u32 *val)
{
        struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
        int ret;

        ret = dw_pcie_read(pci->dbi_base + where, size, val);
        if (ret != PCIBIOS_SUCCESSFUL)
                return ret;

        /*
        * there is a bug of MESON AXG pcie controller that software can not
        * programe PCI_CLASS_DEVICE register, so we must return a fake right
        * value to ensure driver could probe successfully.
        */
        if (where == PCI_CLASS_REVISION && size == 4)
                *val = (PCI_CLASS_BRIDGE_PCI << 16) | (*val & 0xffff);
        else if (where == PCI_CLASS_DEVICE && size == 2)
                *val = PCI_CLASS_BRIDGE_PCI;

        return PCIBIOS_SUCCESSFUL;
}

3, We must ensure class is PCI_CLASS_BRIDGE_PCI except right hdr_type,
   or pci_setup_device() will get failed: 

   ...
   class = pci_class(dev);
   dev->revision = class & 0xff;
   dev->class = class >> 8; /* upper 3 bytes */ 
   ....
   switch (dev->hdr_type) { /* header type */
   ... 
   case PCI_HEADER_TYPE_BRIDGE: /* bridge header */
      if (class != PCI_CLASS_BRIDGE_PCI)  /* class must be PCI_CLASS_BRIDGE_PCI  */
         goto bad; 


thanks.

hanjie

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v6 2/2] PCI: amlogic: Add the Amlogic Meson PCIe controller driver

[Lorenzo Pieralisi]

On Tue, Dec 04, 2018 at 06:40:55PM +0800, Hanjie Lin wrote:
> 
> 
> On 2018/12/4 6:57, Bjorn Helgaas wrote:
> > On Mon, Dec 03, 2018 at 04:41:50PM +0000, Lorenzo Pieralisi wrote:
> >> On Thu, Nov 22, 2018 at 04:53:54PM +0800, Hanjie Lin wrote:
> >>
> >> [...]
> >>
> >>> +static int meson_pcie_rd_own_conf(struct pcie_port *pp, int where, int size,
> >>> +				  u32 *val)
> >>> +{
> >>> +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> >>> +
> >>> +	/*
> >>> +	 * there is a bug of MESON AXG pcie controller that software can not
> >>> +	 * programe PCI_CLASS_DEVICE register, so we must return a fake right
> >>> +	 * value to ensure driver could probe successfully.
> >>> +	 */
> >>> +	if (where == PCI_CLASS_REVISION) {
> >>> +		*val = readl(pci->dbi_base + PCI_CLASS_REVISION);
> >>> +		/* keep revision id */
> >>> +		*val &= PCI_CLASS_REVISION_MASK;
> >>> +		*val |= PCI_CLASS_BRIDGE_PCI << 16;
> >>> +		return PCIBIOS_SUCCESSFUL;
> >>> +	}
> >>
> >> As I said before, this looks broken. If this code (or other drivers with
> >> the same broken assumptions, eg dwc/pcie-qcom.c) carries out a, say,
> >> byte sized config access of eg PCI_CLASS_DEVICE you will get junk out of
> >> it according to your comment above.
> >>
> >> I would like to pick Bjorn's brain on this to see what we can really do
> >> to fix this (and other) drivers.
> > 
> >   - Check to see whether you're reading anything in the 32-bit dword at
> >     offset 0x08.
> > 
> >   - Do the 32-bit readl().
> > 
> >   - Insert the correct Sub-Class and Base Class code (you also throw
> >     away the Programming Interface; not sure why that is)
> > 
> >   - If you're reading something smaller than 32 bits, mask & shift as
> >     needed.  pci_bridge_emul_conf_read() does something similar that
> >     you might be able to copy.
> > 
> > Out of curiosity, what code depends on PCI_CLASS_BRIDGE_PCI?  There
> > are several places in the kernel that currently depend on it, but I
> > think several of them *should* be checking dev->hdr_type to identify a
> > type 1 header instead.
> > 
> > Bjorn
> > 
> > .
> > 
> 
> Yes, it would be broken in particular scenes(eg: read 1 or 2 bytes from 0xa/PCI_CLASS_DEVICE)
> that I didn't considered.
>  
> As your suggestion, I consider some code below may help this issue:
> 1, First call dw_pcie_read() help to read 1/2/4 bytes from register,
>    request all other *size* bytes will return error and dw_pcie_read()
>    will also check register alignment.
>  
> 2, If dw_pcie_read() return success and *where* is 0x8/PCI_CLASS_DEVICE or 0xa/PCI_CLASS_REVISION,
>    we may need to correct class code.
>    As PCI_CLASS_REVISION is two-bytes register, so only when read 4 bytes from 0x8/PCI_CLASS_DEVICE
>    or read 2 bytes from 0xa/PCI_CLASS_REVISION we should correct the class code.
>  
> ps: read 1 byte from 0xa/PCI_CLASS_REVISION or 0xb will get incorrect value.

You can fix this too.

> static int meson_pcie_rd_own_conf(struct pcie_port *pp, int where, int size,
>                 u32 *val)
> {
>         struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>         int ret;
> 
>         ret = dw_pcie_read(pci->dbi_base + where, size, val);
>         if (ret != PCIBIOS_SUCCESSFUL)
>                 return ret;
> 
>         /*
>         * there is a bug of MESON AXG pcie controller that software can not
>         * programe PCI_CLASS_DEVICE register, so we must return a fake right

"There is a bug in the MESON AXG pcie controller whereby software cannot
programme the PCI_CLASS_DEVICE register, so we must fabricate the return
value in the config accessors."

>         * value to ensure driver could probe successfully.
>         */
>         if (where == PCI_CLASS_REVISION && size == 4)
>                 *val = (PCI_CLASS_BRIDGE_PCI << 16) | (*val & 0xffff);
>         else if (where == PCI_CLASS_DEVICE && size == 2)
>                 *val = PCI_CLASS_BRIDGE_PCI;

You can further filter it with (where & 0x1) == PCI_CLASS_DEVICE
and handle the size accordingly, so that even a byte access would
work, for completeness.

Lorenzo

>         return PCIBIOS_SUCCESSFUL;
> }
> 
> 3, We must ensure class is PCI_CLASS_BRIDGE_PCI except right hdr_type,
>    or pci_setup_device() will get failed: 
> 
>    ...
>    class = pci_class(dev);
>    dev->revision = class & 0xff;
>    dev->class = class >> 8; /* upper 3 bytes */ 
>    ....
>    switch (dev->hdr_type) { /* header type */
>    ... 
>    case PCI_HEADER_TYPE_BRIDGE: /* bridge header */
>       if (class != PCI_CLASS_BRIDGE_PCI)  /* class must be PCI_CLASS_BRIDGE_PCI  */
>          goto bad; 
> 
> 
> thanks.
> 
> hanjie

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v6 2/2] PCI: amlogic: Add the Amlogic Meson PCIe controller driver

[Hanjie Lin]

On 2018/12/4 20:00, Lorenzo Pieralisi wrote:
> On Tue, Dec 04, 2018 at 06:40:55PM +0800, Hanjie Lin wrote:
>>
>>
>> On 2018/12/4 6:57, Bjorn Helgaas wrote:
>>> On Mon, Dec 03, 2018 at 04:41:50PM +0000, Lorenzo Pieralisi wrote:
>>>> On Thu, Nov 22, 2018 at 04:53:54PM +0800, Hanjie Lin wrote:
>>>>
>>>> [...]
>>>>
>>>>> +static int meson_pcie_rd_own_conf(struct pcie_port *pp, int where, int size,
>>>>> +				  u32 *val)
>>>>> +{
>>>>> +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>>>>> +
>>>>> +	/*
>>>>> +	 * there is a bug of MESON AXG pcie controller that software can not
>>>>> +	 * programe PCI_CLASS_DEVICE register, so we must return a fake right
>>>>> +	 * value to ensure driver could probe successfully.
>>>>> +	 */
>>>>> +	if (where == PCI_CLASS_REVISION) {
>>>>> +		*val = readl(pci->dbi_base + PCI_CLASS_REVISION);
>>>>> +		/* keep revision id */
>>>>> +		*val &= PCI_CLASS_REVISION_MASK;
>>>>> +		*val |= PCI_CLASS_BRIDGE_PCI << 16;
>>>>> +		return PCIBIOS_SUCCESSFUL;
>>>>> +	}
>>>>
>>>> As I said before, this looks broken. If this code (or other drivers with
>>>> the same broken assumptions, eg dwc/pcie-qcom.c) carries out a, say,
>>>> byte sized config access of eg PCI_CLASS_DEVICE you will get junk out of
>>>> it according to your comment above.
>>>>
>>>> I would like to pick Bjorn's brain on this to see what we can really do
>>>> to fix this (and other) drivers.
>>>
>>>   - Check to see whether you're reading anything in the 32-bit dword at
>>>     offset 0x08.
>>>
>>>   - Do the 32-bit readl().
>>>
>>>   - Insert the correct Sub-Class and Base Class code (you also throw
>>>     away the Programming Interface; not sure why that is)
>>>
>>>   - If you're reading something smaller than 32 bits, mask & shift as
>>>     needed.  pci_bridge_emul_conf_read() does something similar that
>>>     you might be able to copy.
>>>
>>> Out of curiosity, what code depends on PCI_CLASS_BRIDGE_PCI?  There
>>> are several places in the kernel that currently depend on it, but I
>>> think several of them *should* be checking dev->hdr_type to identify a
>>> type 1 header instead.
>>>
>>> Bjorn
>>>
>>> .
>>>
>>
>> Yes, it would be broken in particular scenes(eg: read 1 or 2 bytes from 0xa/PCI_CLASS_DEVICE)
>> that I didn't considered.
>>  
>> As your suggestion, I consider some code below may help this issue:
>> 1, First call dw_pcie_read() help to read 1/2/4 bytes from register,
>>    request all other *size* bytes will return error and dw_pcie_read()
>>    will also check register alignment.
>>  
>> 2, If dw_pcie_read() return success and *where* is 0x8/PCI_CLASS_DEVICE or 0xa/PCI_CLASS_REVISION,
>>    we may need to correct class code.
>>    As PCI_CLASS_REVISION is two-bytes register, so only when read 4 bytes from 0x8/PCI_CLASS_DEVICE
>>    or read 2 bytes from 0xa/PCI_CLASS_REVISION we should correct the class code.
>>  
>> ps: read 1 byte from 0xa/PCI_CLASS_REVISION or 0xb will get incorrect value.
> 
> You can fix this too.
> 
>> static int meson_pcie_rd_own_conf(struct pcie_port *pp, int where, int size,
>>                 u32 *val)
>> {
>>         struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>>         int ret;
>>
>>         ret = dw_pcie_read(pci->dbi_base + where, size, val);
>>         if (ret != PCIBIOS_SUCCESSFUL)
>>                 return ret;
>>
>>         /*
>>         * there is a bug of MESON AXG pcie controller that software can not
>>         * programe PCI_CLASS_DEVICE register, so we must return a fake right
> 
> "There is a bug in the MESON AXG pcie controller whereby software cannot
> programme the PCI_CLASS_DEVICE register, so we must fabricate the return
> value in the config accessors."
> 

Ok, it's more clear :)

>>         * value to ensure driver could probe successfully.
>>         */
>>         if (where == PCI_CLASS_REVISION && size == 4)
>>                 *val = (PCI_CLASS_BRIDGE_PCI << 16) | (*val & 0xffff);
>>         else if (where == PCI_CLASS_DEVICE && size == 2)
>>                 *val = PCI_CLASS_BRIDGE_PCI;
> 
> You can further filter it with (where & 0x1) == PCI_CLASS_DEVICE
> and handle the size accordingly, so that even a byte access would
> work, for completeness.
> 
> Lorenzo
>

Of course, I will add process to handle one-byte access from 0xa/0xb register 
in next-version.

thanks.

hanjie
 
>>         return PCIBIOS_SUCCESSFUL;
>> }
>>
>> 3, We must ensure class is PCI_CLASS_BRIDGE_PCI except right hdr_type,
>>    or pci_setup_device() will get failed: 
>>
>>    ...
>>    class = pci_class(dev);
>>    dev->revision = class & 0xff;
>>    dev->class = class >> 8; /* upper 3 bytes */ 
>>    ....
>>    switch (dev->hdr_type) { /* header type */
>>    ... 
>>    case PCI_HEADER_TYPE_BRIDGE: /* bridge header */
>>       if (class != PCI_CLASS_BRIDGE_PCI)  /* class must be PCI_CLASS_BRIDGE_PCI  */
>>          goto bad; 
>>
>>
>> thanks.
>>
>> hanjie
> 
> .
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel