[v4,2/3] PCI: mediatek: Add new generation controller support

MediaTek's PCIe host controller has three generation HWs, the new
generation HW is an individual bridge, it supoorts Gen3 speed and
up to 256 MSI interrupt numbers for multi-function devices.

Add support for new Gen3 controller which can be found on MT8192.

Signed-off-by: Jianjun Wang <jianjun.wang@mediatek.com>
Acked-by: Ryder Lee <ryder.lee@mediatek.com>
---
 drivers/pci/controller/Kconfig              |   14 +
 drivers/pci/controller/Makefile             |    1 +
 drivers/pci/controller/pcie-mediatek-gen3.c | 1031 +++++++++++++++++++
 3 files changed, 1046 insertions(+)
 create mode 100644 drivers/pci/controller/pcie-mediatek-gen3.c

On Wed, Nov 18, 2020 at 04:29:34PM +0800, Jianjun Wang wrote:
> MediaTek's PCIe host controller has three generation HWs, the new

> generation HW is an individual bridge, it supoorts Gen3 speed and

> up to 256 MSI interrupt numbers for multi-function devices.

> 

> Add support for new Gen3 controller which can be found on MT8192.

> 

> Signed-off-by: Jianjun Wang <jianjun.wang@mediatek.com>

> Acked-by: Ryder Lee <ryder.lee@mediatek.com>

> ---

>  drivers/pci/controller/Kconfig              |   14 +

>  drivers/pci/controller/Makefile             |    1 +

>  drivers/pci/controller/pcie-mediatek-gen3.c | 1031 +++++++++++++++++++

>  3 files changed, 1046 insertions(+)

>  create mode 100644 drivers/pci/controller/pcie-mediatek-gen3.c

> 

> diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig

> index f18c3725ef80..83daa772595b 100644

> --- a/drivers/pci/controller/Kconfig

> +++ b/drivers/pci/controller/Kconfig

> @@ -239,6 +239,20 @@ config PCIE_MEDIATEK

>  	  Say Y here if you want to enable PCIe controller support on

>  	  MediaTek SoCs.

>  

> +config PCIE_MEDIATEK_GEN3

> +	tristate "MediaTek GEN3 PCIe controller"

> +	depends on ARCH_MEDIATEK || COMPILE_TEST

> +	depends on OF

> +	depends on PCI_MSI_IRQ_DOMAIN

> +	help

> +	  Adds support for PCIe Gen3 MAC controller for MediaTek SoCs.

> +	  This PCIe controller provides the capable of Gen3, Gen2 and

> +	  Gen1 speed, and support up to 256 MSI interrupt numbers for

> +	  multi-function devices.

> +

> +	  Say Y here if you want to enable Gen3 PCIe controller support on

> +	  MediaTek SoCs.

> +

>  config PCIE_TANGO_SMP8759

>  	bool "Tango SMP8759 PCIe controller (DANGEROUS)"

>  	depends on ARCH_TANGO && PCI_MSI && OF

> diff --git a/drivers/pci/controller/Makefile b/drivers/pci/controller/Makefile

> index bcdbf49ab1e4..9c1b96777597 100644

> --- a/drivers/pci/controller/Makefile

> +++ b/drivers/pci/controller/Makefile

> @@ -27,6 +27,7 @@ obj-$(CONFIG_PCIE_ROCKCHIP) += pcie-rockchip.o

>  obj-$(CONFIG_PCIE_ROCKCHIP_EP) += pcie-rockchip-ep.o

>  obj-$(CONFIG_PCIE_ROCKCHIP_HOST) += pcie-rockchip-host.o

>  obj-$(CONFIG_PCIE_MEDIATEK) += pcie-mediatek.o

> +obj-$(CONFIG_PCIE_MEDIATEK_GEN3) += pcie-mediatek-gen3.o

>  obj-$(CONFIG_PCIE_TANGO_SMP8759) += pcie-tango.o

>  obj-$(CONFIG_VMD) += vmd.o

>  obj-$(CONFIG_PCIE_BRCMSTB) += pcie-brcmstb.o

> diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c b/drivers/pci/controller/pcie-mediatek-gen3.c

> new file mode 100644

> index 000000000000..dd454cfee373

> --- /dev/null

> +++ b/drivers/pci/controller/pcie-mediatek-gen3.c

> @@ -0,0 +1,1031 @@

> +// SPDX-License-Identifier: GPL-2.0

> +/*

> + * MediaTek PCIe host controller driver.

> + *

> + * Copyright (c) 2020 MediaTek Inc.

> + * Author: Jianjun Wang <jianjun.wang@mediatek.com>

> + */

> +

> +#include <linux/clk.h>

> +#include <linux/delay.h>

> +#include <linux/iopoll.h>

> +#include <linux/irq.h>

> +#include <linux/irqchip/chained_irq.h>

> +#include <linux/irqdomain.h>

> +#include <linux/kernel.h>

> +#include <linux/module.h>

> +#include <linux/msi.h>

> +#include <linux/of_address.h>

> +#include <linux/of_clk.h>

> +#include <linux/of_pci.h>

> +#include <linux/of_platform.h>

> +#include <linux/pci.h>

> +#include <linux/phy/phy.h>

> +#include <linux/platform_device.h>

> +#include <linux/pm_domain.h>

> +#include <linux/pm_runtime.h>

> +#include <linux/reset.h>

> +

> +#include "../pci.h"

> +

> +#define PCIE_SETTING_REG		0x80

> +#define PCIE_PCI_IDS_1			0x9c

> +#define PCI_CLASS(class)		(class << 8)

> +#define PCIE_RC_MODE			BIT(0)

> +

> +#define PCIE_CFGNUM_REG			0x140

> +#define PCIE_CFG_DEVFN(devfn)		((devfn) & GENMASK(7, 0))

> +#define PCIE_CFG_BUS(bus)		(((bus) << 8) & GENMASK(15, 8))

> +#define PCIE_CFG_BYTE_EN(bytes)		(((bytes) << 16) & GENMASK(19, 16))

> +#define PCIE_CFG_FORCE_BYTE_EN		BIT(20)

> +#define PCIE_CFG_OFFSET_ADDR		0x1000

> +#define PCIE_CFG_HEADER(devfn, bus) \

> +	(PCIE_CFG_DEVFN(devfn) | PCIE_CFG_BUS(bus))

> +

> +#define PCIE_CFG_HEADER_FORCE_BE(devfn, bus, bytes) \

> +	(PCIE_CFG_HEADER(devfn, bus) | PCIE_CFG_BYTE_EN(bytes) \

> +	 | PCIE_CFG_FORCE_BYTE_EN)

> +

> +#define PCIE_RST_CTRL_REG		0x148

> +#define PCIE_MAC_RSTB			BIT(0)

> +#define PCIE_PHY_RSTB			BIT(1)

> +#define PCIE_BRG_RSTB			BIT(2)

> +#define PCIE_PE_RSTB			BIT(3)

> +

> +#define PCIE_MISC_STATUS_REG		0x14C

> +#define PCIE_LTR_MSG_RECEIVED		BIT(0)

> +#define PCIE_PCIE_MSG_RECEIVED		BIT(1)

> +

> +#define PCIE_LTSSM_STATUS_REG		0x150

> +#define PCIE_LTSSM_STATE_MASK		GENMASK(28, 24)

> +#define PCIE_LTSSM_STATE(val)		((val & PCIE_LTSSM_STATE_MASK) >> 24)

> +#define PCIE_LTSSM_STATE_L0		0x10

> +#define PCIE_LTSSM_STATE_L1_IDLE	0x13

> +#define PCIE_LTSSM_STATE_L2_IDLE	0x14

> +

> +#define PCIE_LINK_STATUS_REG		0x154

> +#define PCIE_PORT_LINKUP		BIT(8)

> +

> +#define PCIE_MSI_SET_NUM		8

> +#define PCIE_MSI_IRQS_PER_SET		32

> +#define PCIE_MSI_IRQS_NUM \

> +	(PCIE_MSI_IRQS_PER_SET * (PCIE_MSI_SET_NUM))

> +

> +#define PCIE_INT_ENABLE_REG		0x180

> +#define PCIE_MSI_MASK			GENMASK(PCIE_MSI_SET_NUM + 8 - 1, 8)

> +#define PCIE_MSI_SHIFT			8

> +#define PCIE_INTX_SHIFT			24

> +#define PCIE_INTX_MASK			GENMASK(27, 24)

> +#define PCIE_MSG_MASK			BIT(28)

> +#define PCIE_AER_MASK			BIT(29)

> +#define PCIE_PM_MASK			BIT(30)

> +

> +#define PCIE_INT_STATUS_REG		0x184

> +#define PCIE_MSI_SET_ENABLE_REG		0x190

> +

> +#define PCIE_LOW_POWER_CTRL_REG		0x194

> +#define PCIE_DIS_LOWPWR_MASK		GENMASK(3, 0)

> +#define PCIE_DIS_L0S_MASK		BIT(0)

> +#define PCIE_DIS_L1_MASK		BIT(1)

> +#define PCIE_DIS_L11_MASK		BIT(2)

> +#define PCIE_DIS_L12_MASK		BIT(3)

> +#define PCIE_FORCE_DIS_LOWPWR		GENMASK(11, 8)

> +#define PCIE_FORCE_DIS_L0S		BIT(8)

> +#define PCIE_FORCE_DIS_L1		BIT(9)

> +#define PCIE_FORCE_DIS_L11		BIT(10)

> +#define PCIE_FORCE_DIS_L12		BIT(11)

> +

> +#define PCIE_ICMD_PM_REG		0x198

> +#define PCIE_TURN_OFF_LINK		BIT(4)

> +

> +#define PCIE_MSI_ADDR_BASE_REG		0xc00

> +#define PCIE_MSI_SET_OFFSET		0x10

> +#define PCIE_MSI_STATUS_OFFSET		0x04

> +#define PCIE_MSI_ENABLE_OFFSET		0x08

> +

> +#define PCIE_TRANS_TABLE_BASE_REG	0x800

> +#define PCIE_ATR_SRC_ADDR_MSB_OFFSET	0x4

> +#define PCIE_ATR_TRSL_ADDR_LSB_OFFSET	0x8

> +#define PCIE_ATR_TRSL_ADDR_MSB_OFFSET	0xc

> +#define PCIE_ATR_TRSL_PARAM_OFFSET	0x10

> +#define PCIE_ATR_TLB_SET_OFFSET		0x20

> +

> +#define PCIE_MAX_TRANS_TABLES		8

> +#define ATR_EN				BIT(0)

> +#define ATR_SIZE(size)			((((size) - 1) << 1) & GENMASK(6, 1))

> +#define ATR_ID(id)			(id & GENMASK(3, 0))

> +#define ATR_PARAM(param)		(((param) << 16) & GENMASK(27, 16))

> +

> +/**

> + * struct mtk_pcie_msi - MSI information for each set

> + * @base: IO mapped register base

> + * @irq: MSI set Interrupt number

> + * @index: MSI set number

> + * @msg_addr: MSI message address

> + * @domain: IRQ domain

> + */

> +struct mtk_pcie_msi {

> +	void __iomem *base;

> +	unsigned int irq;

> +	int index;

> +	phys_addr_t msg_addr;

> +	struct irq_domain *domain;

> +};

> +

> +/**

> + * struct mtk_pcie_port - PCIe port information

> + * @dev: PCIe device

> + * @base: IO mapped register base

> + * @reg_base: Physical register base

> + * @mac_reset: mac reset control

> + * @phy_reset: phy reset control

> + * @phy: PHY controller block

> + * @clks: PCIe clocks

> + * @num_clks: PCIe clocks count for this port

> + * @is_suspended: device suspend state

> + * @irq: PCIe controller interrupt number

> + * @intx_domain: legacy INTx IRQ domain

> + * @msi_domain: MSI IRQ domain

> + * @msi_top_domain: MSI IRQ top domain

> + * @msi_info: MSI sets information

> + * @lock: lock protecting IRQ bit map

> + * @msi_irq_in_use: bit map for assigned MSI IRQ

> + */

> +struct mtk_pcie_port {

> +	struct device *dev;

> +	void __iomem *base;

> +	phys_addr_t reg_base;

> +	struct reset_control *mac_reset;

> +	struct reset_control *phy_reset;

> +	struct phy *phy;

> +	struct clk_bulk_data *clks;

> +	int num_clks;

> +	bool is_suspended;

> +

> +	int irq;

> +	struct irq_domain *intx_domain;

> +	struct irq_domain *msi_domain;

> +	struct irq_domain *msi_top_domain;

> +	struct mtk_pcie_msi **msi_info;

> +	struct mutex lock;

> +	DECLARE_BITMAP(msi_irq_in_use, PCIE_MSI_IRQS_NUM);

> +};

> +

> +static int mtk_pcie_config_read(struct pci_bus *bus, unsigned int devfn,

> +				    int where, int size, u32 *val)

> +{

> +	struct mtk_pcie_port *port = bus->sysdata;

> +	int bytes;

> +

> +	bytes = ((1 << size) - 1) << (where & 0x3);

> +	writel(PCIE_CFG_HEADER_FORCE_BE(devfn, bus->number, bytes),

> +	       port->base + PCIE_CFGNUM_REG);


Put this in a map_bus() function and then use pci_generic_config_read32 
and _write32.

32-bit only accesses are problematic. Do you require 32-bit accesses for 
downstream devices or just root port/bus? You can now define child_ops. 

> +

> +	*val = readl(port->base + PCIE_CFG_OFFSET_ADDR + (where & ~0x3));

> +

> +	if (size <= 2)

> +		*val = (*val >> (8 * (where & 0x3))) & ((1 << (size * 8)) - 1);

> +

> +	return PCIBIOS_SUCCESSFUL;

> +}

> +

> +static int mtk_pcie_config_write(struct pci_bus *bus, unsigned int devfn,

> +				     int where, int size, u32 val)

> +{

> +	struct mtk_pcie_port *port = bus->sysdata;

> +	int bytes;

> +

> +	bytes = ((1 << size) - 1) << (where & 0x3);

> +	writel(PCIE_CFG_HEADER_FORCE_BE(devfn, bus->number, bytes),

> +	       port->base + PCIE_CFGNUM_REG);

> +

> +	if (size <= 2)

> +		val = (val & ((1 << (size * 8)) - 1)) << ((where & 0x3) * 8);

> +

> +	writel(val, port->base + PCIE_CFG_OFFSET_ADDR + (where & ~0x3));

> +

> +	return PCIBIOS_SUCCESSFUL;

> +}

> +

> +static struct pci_ops mtk_pcie_ops = {

> +	.read  = mtk_pcie_config_read,

> +	.write = mtk_pcie_config_write,

> +};

> +

> +static void mtk_pcie_set_trans_window(void __iomem *reg,

> +				      resource_size_t cpu_addr,

> +				      resource_size_t pci_addr, size_t size)

> +{

> +	writel(lower_32_bits(cpu_addr) | ATR_SIZE(fls(size) - 1) | ATR_EN, reg);

> +	writel(upper_32_bits(cpu_addr), reg + PCIE_ATR_SRC_ADDR_MSB_OFFSET);

> +	writel(lower_32_bits(pci_addr), reg + PCIE_ATR_TRSL_ADDR_LSB_OFFSET);

> +	writel(upper_32_bits(pci_addr), reg + PCIE_ATR_TRSL_ADDR_MSB_OFFSET);

> +	writel(ATR_ID(0) | ATR_PARAM(0), reg + PCIE_ATR_TRSL_PARAM_OFFSET);

> +}

> +

> +static int mtk_pcie_set_trans_table(void __iomem *reg,

> +				    resource_size_t cpu_addr,

> +				    resource_size_t pci_addr, size_t size,

> +				    int num)


Just merge these 2 functions into 1.

> +{

> +	void __iomem *table_base;

> +

> +	if (num > PCIE_MAX_TRANS_TABLES)

> +		return -ENODEV;

> +

> +	table_base = reg + num * PCIE_ATR_TLB_SET_OFFSET;

> +	mtk_pcie_set_trans_window(table_base, cpu_addr, pci_addr, size);

> +

> +	return 0;

> +}

> +

> +static int mtk_pcie_startup_port(struct mtk_pcie_port *port)

> +{

> +	struct resource_entry *entry;

> +	struct pci_host_bridge *host = pci_host_bridge_from_priv(port);

> +	unsigned int table_index = 0;

> +	int err;

> +	u32 val;

> +

> +	/* Set as RC mode */

> +	val = readl(port->base + PCIE_SETTING_REG);

> +	val |= PCIE_RC_MODE;

> +	writel(val, port->base + PCIE_SETTING_REG);

> +

> +	/* Set class code */

> +	val = readl(port->base + PCIE_PCI_IDS_1);

> +	val &= ~GENMASK(31, 8);

> +	val |= PCI_CLASS(PCI_CLASS_BRIDGE_PCI << 8);

> +	writel(val, port->base + PCIE_PCI_IDS_1);

> +

> +	/* Assert all reset signals */

> +	val = readl(port->base + PCIE_RST_CTRL_REG);

> +	val |= PCIE_MAC_RSTB | PCIE_PHY_RSTB | PCIE_BRG_RSTB | PCIE_PE_RSTB;

> +	writel(val, port->base + PCIE_RST_CTRL_REG);

> +

> +	/* De-assert reset signals*/

> +	val &= ~(PCIE_MAC_RSTB | PCIE_PHY_RSTB | PCIE_BRG_RSTB);

> +	writel(val, port->base + PCIE_RST_CTRL_REG);

> +

> +	/* Delay 100ms to wait the reference clocks become stable */

> +	usleep_range(100 * 1000, 120 * 1000);

> +

> +	/* De-assert pe reset*/

> +	val &= ~PCIE_PE_RSTB;

> +	writel(val, port->base + PCIE_RST_CTRL_REG);

> +

> +	/* Check if the link is up or not */

> +	err = readl_poll_timeout(port->base + PCIE_LINK_STATUS_REG, val,

> +			!!(val & PCIE_PORT_LINKUP), 20,

> +			50 * USEC_PER_MSEC);

> +	if (err) {

> +		val = readl(port->base + PCIE_LTSSM_STATUS_REG);

> +		dev_dbg(port->dev, "ltssm reg val: %#x\n", val);

> +		return err;

> +	}

> +

> +	/* Set PCIe translation windows */

> +	resource_list_for_each_entry(entry, &host->windows) {

> +		unsigned long type = resource_type(entry->res);

> +		struct resource *res = NULL;

> +		resource_size_t cpu_addr;

> +		resource_size_t pci_addr;

> +

> +		if (!(type & (IORESOURCE_MEM | IORESOURCE_IO)))

> +			continue;

> +

> +		res = entry->res;

> +		cpu_addr = res->start;

> +		pci_addr = res->start - entry->offset;

> +		mtk_pcie_set_trans_table(port->base + PCIE_TRANS_TABLE_BASE_REG,

> +					 cpu_addr, pci_addr, resource_size(res),

> +					 table_index);


You don't distinguish IO regions in any way. How can that work?

> +

> +		dev_dbg(port->dev, "Set %s trans window[%d]: cpu_addr = %#llx, pci_addr = %#llx, size = %#llx\n",

> +			(!!(type & IORESOURCE_MEM) ? "MEM" : "IO"), table_index,

> +			cpu_addr, pci_addr, resource_size(res));

> +

> +		table_index++;

> +	}

> +

> +	return 0;

> +}

> +

> +static inline struct mtk_pcie_msi *mtk_get_msi_info(struct mtk_pcie_port *port,

> +						    unsigned long hwirq)

> +{

> +	return port->msi_info[hwirq / PCIE_MSI_IRQS_PER_SET];

> +}

> +

> +static int mtk_pcie_set_affinity(struct irq_data *data,

> +				 const struct cpumask *mask, bool force)

> +{

> +	struct mtk_pcie_port *port = irq_data_get_irq_chip_data(data);

> +	int ret;

> +

> +	ret = irq_set_affinity_hint(port->irq, mask);

> +	if (ret)

> +		return ret;

> +

> +	irq_data_update_effective_affinity(data, mask);

> +

> +	return 0;

> +}

> +

> +static void mtk_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)

> +{

> +	struct mtk_pcie_msi *msi_info;

> +	struct mtk_pcie_port *port = irq_data_get_irq_chip_data(data);

> +

> +	msi_info = mtk_get_msi_info(port, data->hwirq);

> +

> +	msg->address_hi = 0;

> +	msg->address_lo = lower_32_bits(msi_info->msg_addr);

> +

> +	msg->data = data->hwirq;

> +	dev_dbg(port->dev, "msi#%#lx address_hi %#x address_lo %#x data %d\n",

> +		data->hwirq, msg->address_hi, msg->address_lo, msg->data);

> +}

> +

> +static void mtk_msi_irq_ack(struct irq_data *data)

> +{

> +	struct mtk_pcie_msi *msi_info;

> +	struct mtk_pcie_port *port = irq_data_get_irq_chip_data(data);

> +	unsigned long hwirq;

> +

> +	msi_info = mtk_get_msi_info(port, data->hwirq);

> +

> +	hwirq =	data->hwirq % PCIE_MSI_IRQS_PER_SET;

> +

> +	writel(1 << hwirq, msi_info->base + PCIE_MSI_STATUS_OFFSET);

> +}

> +

> +static void mtk_msi_irq_mask(struct irq_data *data)

> +{

> +	struct mtk_pcie_msi *msi_info;

> +	struct mtk_pcie_port *port = irq_data_get_irq_chip_data(data);

> +	u32 val;

> +

> +	msi_info = mtk_get_msi_info(port, data->hwirq);

> +	val = readl(msi_info->base + PCIE_MSI_ENABLE_OFFSET);

> +	val &= ~(1 << data->hwirq);


Use BIT() macro here and elsewhere.

> +	writel(val, msi_info->base + PCIE_MSI_ENABLE_OFFSET);

> +

> +	pci_msi_mask_irq(data);

> +}

> +

> +static void mtk_msi_irq_unmask(struct irq_data *data)

> +{

> +	struct mtk_pcie_msi *msi_info;

> +	struct mtk_pcie_port *port = irq_data_get_irq_chip_data(data);

> +	u32 val;

> +

> +	msi_info = mtk_get_msi_info(port, data->hwirq);

> +	val = readl(msi_info->base + PCIE_MSI_ENABLE_OFFSET);

> +	val |= (1 << data->hwirq);

> +	writel(val, msi_info->base + PCIE_MSI_ENABLE_OFFSET);

> +

> +	pci_msi_unmask_irq(data);

> +}

> +

> +static struct irq_chip mtk_msi_irq_chip = {

> +	.irq_ack		= mtk_msi_irq_ack,

> +	.irq_compose_msi_msg	= mtk_compose_msi_msg,

> +	.irq_mask		= mtk_msi_irq_mask,

> +	.irq_unmask		= mtk_msi_irq_unmask,

> +	.irq_set_affinity	= mtk_pcie_set_affinity,

> +	.name			= "PCIe",

> +};

> +

> +static irq_hw_number_t mtk_pcie_msi_get_hwirq(struct msi_domain_info *info,

> +					      msi_alloc_info_t *arg)

> +{

> +	struct msi_desc *entry = arg->desc;

> +	struct mtk_pcie_port *port = info->chip_data;

> +	int hwirq;

> +

> +	mutex_lock(&port->lock);

> +

> +	hwirq = bitmap_find_free_region(port->msi_irq_in_use, PCIE_MSI_IRQS_NUM,

> +			order_base_2(entry->nvec_used));

> +	if (hwirq < 0) {

> +		mutex_unlock(&port->lock);

> +		return -ENOSPC;

> +	}

> +

> +	mutex_unlock(&port->lock);

> +

> +	return hwirq;

> +}

> +

> +static void mtk_pcie_msi_free(struct irq_domain *domain,

> +			      struct msi_domain_info *info, unsigned int virq)

> +{

> +	struct irq_data *data = irq_domain_get_irq_data(domain, virq);

> +	struct mtk_pcie_port *port = irq_data_get_irq_chip_data(data);

> +

> +	mutex_lock(&port->lock);

> +

> +	bitmap_clear(port->msi_irq_in_use, data->hwirq, 1);

> +

> +	mutex_unlock(&port->lock);

> +}

> +

> +static struct msi_domain_ops mtk_msi_domain_ops = {

> +	.get_hwirq	= mtk_pcie_msi_get_hwirq,

> +	.msi_free	= mtk_pcie_msi_free,

> +};

> +

> +static struct msi_domain_info mtk_msi_domain_info = {

> +	.flags		= (MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_PCI_MSIX |

> +			   MSI_FLAG_USE_DEF_CHIP_OPS | MSI_FLAG_MULTI_PCI_MSI),

> +	.chip		= &mtk_msi_irq_chip,

> +	.ops		= &mtk_msi_domain_ops,

> +	.handler	= handle_edge_irq,

> +	.handler_name	= "MSI",

> +};

> +

> +static void mtk_msi_top_irq_eoi(struct irq_data *data)

> +{

> +	struct mtk_pcie_port *port = irq_data_get_irq_chip_data(data);

> +	unsigned long msi_irq = data->hwirq + PCIE_MSI_SHIFT;

> +

> +	writel(1 << msi_irq, port->base + PCIE_INT_STATUS_REG);

> +}

> +

> +static struct irq_chip mtk_msi_top_irq_chip = {

> +	.irq_eoi	= mtk_msi_top_irq_eoi,

> +	.name		= "PCIe",

> +};

> +

> +static void mtk_pcie_msi_handler(struct irq_desc *desc)

> +{

> +	struct mtk_pcie_msi *msi_info = irq_desc_get_handler_data(desc);

> +	struct irq_chip *irqchip = irq_desc_get_chip(desc);

> +	unsigned long msi_enable, msi_status;

> +	unsigned int virq;

> +	irq_hw_number_t bit, hwirq;

> +

> +	chained_irq_enter(irqchip, desc);

> +

> +	msi_enable = readl(msi_info->base + PCIE_MSI_ENABLE_OFFSET);

> +	while ((msi_status = readl(msi_info->base + PCIE_MSI_STATUS_OFFSET))) {

> +		msi_status &= msi_enable;

> +		for_each_set_bit(bit, &msi_status, PCIE_MSI_IRQS_PER_SET) {

> +			hwirq = bit + msi_info->index * PCIE_MSI_IRQS_PER_SET;

> +			virq = irq_find_mapping(msi_info->domain, hwirq);

> +			generic_handle_irq(virq);

> +		}

> +	}

> +

> +	chained_irq_exit(irqchip, desc);

> +}

> +

> +static int mtk_msi_top_domain_map(struct irq_domain *domain,

> +				    unsigned int virq, irq_hw_number_t hwirq)

> +{

> +	struct mtk_pcie_port *port = domain->host_data;

> +	struct mtk_pcie_msi *msi_info = port->msi_info[hwirq];

> +

> +	irq_domain_set_info(domain, virq, hwirq,

> +			    &mtk_msi_top_irq_chip, domain->host_data,

> +			    mtk_pcie_msi_handler, msi_info, NULL);

> +

> +	return 0;

> +}

> +

> +static const struct irq_domain_ops mtk_msi_top_domain_ops = {

> +	.map = mtk_msi_top_domain_map,

> +};

> +

> +static void mtk_intx_mask(struct irq_data *data)

> +{

> +	struct mtk_pcie_port *port = irq_data_get_irq_chip_data(data);

> +	u32 val;

> +

> +	val = readl(port->base + PCIE_INT_ENABLE_REG);

> +	val &= ~(1 << (data->hwirq + PCIE_INTX_SHIFT));

> +	writel(val, port->base + PCIE_INT_ENABLE_REG);

> +}

> +

> +static void mtk_intx_unmask(struct irq_data *data)

> +{

> +	struct mtk_pcie_port *port = irq_data_get_irq_chip_data(data);

> +	u32 val;

> +

> +	val = readl(port->base + PCIE_INT_ENABLE_REG);

> +	val |= 1 << (data->hwirq + PCIE_INTX_SHIFT);

> +	writel(val, port->base + PCIE_INT_ENABLE_REG);

> +}

> +

> +static void mtk_intx_eoi(struct irq_data *data)

> +{

> +	struct mtk_pcie_port *port = irq_data_get_irq_chip_data(data);

> +	unsigned long hwirq;

> +

> +	/**

> +	 * As an emulated level irq, its interrupt status will be remained

> +	 * until receive the corresponding message of de-assert, hence that

> +	 * the status can only be cleared when the interrupt has been serviced.

> +	 */

> +	hwirq = data->hwirq + PCIE_INTX_SHIFT;

> +	writel(1 << hwirq, port->base + PCIE_INT_STATUS_REG);

> +}

> +

> +static struct irq_chip mtk_intx_irq_chip = {

> +	.irq_mask		= mtk_intx_mask,

> +	.irq_unmask		= mtk_intx_unmask,

> +	.irq_eoi		= mtk_intx_eoi,

> +	.irq_set_affinity	= mtk_pcie_set_affinity,

> +	.name			= "PCIe",

> +};

> +

> +static int mtk_pcie_intx_map(struct irq_domain *domain, unsigned int irq,

> +			     irq_hw_number_t hwirq)

> +{

> +	irq_set_chip_and_handler_name(irq, &mtk_intx_irq_chip,

> +				      handle_fasteoi_irq, "INTx");

> +	irq_set_chip_data(irq, domain->host_data);

> +

> +	return 0;

> +}

> +

> +static const struct irq_domain_ops intx_domain_ops = {

> +	.map = mtk_pcie_intx_map,

> +};

> +

> +static int mtk_pcie_init_irq_domains(struct mtk_pcie_port *port,

> +				     struct device_node *node)

> +{

> +	struct device *dev = port->dev;

> +	struct device_node *intc_node;

> +	struct fwnode_handle *fwnode = of_node_to_fwnode(node);

> +	struct mtk_pcie_msi *msi_info;

> +	struct msi_domain_info *info;

> +	int i, ret;

> +

> +	/* Setup INTx */

> +	intc_node = of_get_child_by_name(node, "interrupt-controller");

> +	if (!intc_node) {

> +		dev_notice(dev, "Missing PCIe Intc node\n");

> +		return -ENODEV;

> +	}

> +

> +	port->intx_domain = irq_domain_add_linear(intc_node, PCI_NUM_INTX,

> +						  &intx_domain_ops, port);

> +	if (!port->intx_domain) {

> +		dev_notice(dev, "failed to get INTx IRQ domain\n");

> +		return -ENODEV;

> +	}

> +

> +	/* Setup MSI */

> +	mutex_init(&port->lock);

> +

> +	info = devm_kzalloc(dev, sizeof(*info), GFP_KERNEL);

> +	if (!info)

> +		return -ENOMEM;

> +

> +	memcpy(info, &mtk_msi_domain_info, sizeof(*info));

> +	info->chip_data = port;

> +

> +	port->msi_domain = pci_msi_create_irq_domain(fwnode, info, NULL);

> +	if (!port->msi_domain) {

> +		dev_info(dev, "failed to create msi domain\n");

> +		ret = -ENODEV;

> +		goto err_msi_domain;

> +	}

> +

> +	/* Enable MSI and setup pcie domains */

> +	port->msi_top_domain = irq_domain_add_hierarchy(NULL, 0, 0, node,

> +							&mtk_msi_top_domain_ops,

> +							port);

> +	if (!port->msi_top_domain) {

> +		dev_info(dev, "failed to create msi top domain\n");

> +		ret = -ENODEV;

> +		goto err_msi_top_domain;

> +	}

> +

> +	port->msi_info = devm_kzalloc(dev, PCIE_MSI_SET_NUM, GFP_KERNEL);

> +	if (!port->msi_info) {

> +		ret = -ENOMEM;

> +		goto err_msi_info;

> +	}

> +

> +	for (i = 0; i < PCIE_MSI_SET_NUM; i++) {

> +		int offset = i * PCIE_MSI_SET_OFFSET;

> +		u32 val;

> +

> +		msi_info = devm_kzalloc(dev, sizeof(*msi_info), GFP_KERNEL);

> +		if (!msi_info) {

> +			ret = -ENOMEM;

> +			goto err_msi_set;

> +		}

> +

> +		msi_info->base = port->base + PCIE_MSI_ADDR_BASE_REG + offset;

> +		msi_info->msg_addr = port->reg_base + PCIE_MSI_ADDR_BASE_REG +

> +				     offset;

> +

> +		writel(lower_32_bits(msi_info->msg_addr), msi_info->base);

> +

> +		msi_info->index = i;

> +		msi_info->domain = port->msi_domain;

> +

> +		port->msi_info[i] = msi_info;

> +

> +		/* Alloc irq for each msi set */

> +		msi_info->irq = irq_create_mapping(port->msi_top_domain, i);

> +		if (!msi_info->irq) {

> +			dev_info(dev, "allocate msi top irq failed\n");

> +			ret = -ENOSPC;

> +			goto err_msi_set;

> +		}

> +

> +		val = readl(port->base + PCIE_INT_ENABLE_REG);

> +		val |= (1 << (i + PCIE_MSI_SHIFT));

> +		writel(val, port->base + PCIE_INT_ENABLE_REG);

> +

> +		val = readl(port->base + PCIE_MSI_SET_ENABLE_REG);

> +		val |= (1 << i);

> +		writel(val, port->base + PCIE_MSI_SET_ENABLE_REG);

> +	}

> +

> +	return 0;

> +

> +err_msi_set:

> +	while (--i >= 0) {

> +		msi_info = port->msi_info[i];

> +		irq_dispose_mapping(msi_info->irq);

> +	}

> +err_msi_info:

> +	irq_domain_remove(port->msi_top_domain);

> +err_msi_top_domain:

> +	irq_domain_remove(port->msi_domain);

> +err_msi_domain:

> +	irq_domain_remove(port->intx_domain);

> +

> +	return ret;

> +}

> +

> +static void mtk_pcie_irq_teardown(struct mtk_pcie_port *port)

> +{

> +	struct mtk_pcie_msi *msi_info;

> +	int i;

> +

> +	irq_set_chained_handler_and_data(port->irq, NULL, NULL);

> +

> +	if (port->intx_domain)

> +		irq_domain_remove(port->intx_domain);

> +

> +	if (port->msi_domain)

> +		irq_domain_remove(port->msi_domain);

> +

> +	if (port->msi_top_domain) {

> +		for (i = 0; i < PCIE_MSI_SET_NUM; i++) {

> +			msi_info = port->msi_info[i];

> +			irq_dispose_mapping(msi_info->irq);

> +		}

> +

> +		irq_domain_remove(port->msi_top_domain);

> +	}

> +

> +	irq_dispose_mapping(port->irq);

> +}

> +

> +static void mtk_pcie_irq_handler(struct irq_desc *desc)

> +{

> +	struct mtk_pcie_port *port = irq_desc_get_handler_data(desc);

> +	struct irq_chip *irqchip = irq_desc_get_chip(desc);

> +	unsigned long status;

> +	unsigned int virq;

> +	irq_hw_number_t irq_bit = PCIE_INTX_SHIFT;

> +

> +	chained_irq_enter(irqchip, desc);

> +

> +	status = readl(port->base + PCIE_INT_STATUS_REG);

> +	if (status & PCIE_INTX_MASK) {

> +		for_each_set_bit_from(irq_bit, &status, PCI_NUM_INTX +

> +				      PCIE_INTX_SHIFT) {

> +			virq = irq_find_mapping(port->intx_domain,

> +						irq_bit - PCIE_INTX_SHIFT);

> +			generic_handle_irq(virq);

> +		}

> +	}

> +

> +	if (status & PCIE_MSI_MASK) {

> +		irq_bit = PCIE_MSI_SHIFT;

> +		for_each_set_bit_from(irq_bit, &status, PCIE_MSI_SET_NUM +

> +				      PCIE_MSI_SHIFT) {

> +			virq = irq_find_mapping(port->msi_top_domain,

> +						irq_bit - PCIE_MSI_SHIFT);

> +			generic_handle_irq(virq);

> +		}

> +	}

> +

> +	chained_irq_exit(irqchip, desc);

> +}

> +

> +static int mtk_pcie_setup_irq(struct mtk_pcie_port *port,

> +			      struct device_node *node)

> +{

> +	struct device *dev = port->dev;

> +	struct platform_device *pdev = to_platform_device(dev);

> +	int err;

> +

> +	err = mtk_pcie_init_irq_domains(port, node);

> +	if (err) {

> +		dev_notice(dev, "failed to init PCIe IRQ domain\n");

> +		return err;

> +	}

> +

> +	port->irq = platform_get_irq(pdev, 0);

> +	if (port->irq < 0)

> +		return port->irq;

> +

> +	irq_set_chained_handler_and_data(port->irq, mtk_pcie_irq_handler, port);

> +

> +	return 0;

> +}

> +

> +static int mtk_pcie_clk_init(struct mtk_pcie_port *port)

> +{

> +	int ret;

> +

> +	port->num_clks = devm_clk_bulk_get_all(port->dev, &port->clks);

> +	if (port->num_clks < 0) {

> +		dev_notice(port->dev, "failed to get pcie clock\n");

> +		return port->num_clks;

> +	}

> +

> +	ret = clk_bulk_prepare_enable(port->num_clks, port->clks);

> +	if (ret) {

> +		dev_notice(port->dev, "failed to enable pcie clocks\n");

> +		return ret;

> +	}

> +

> +	return 0;

> +}

> +

> +static int mtk_pcie_power_up(struct mtk_pcie_port *port)

> +{

> +	struct device *dev = port->dev;

> +	int err;

> +

> +	port->phy_reset = devm_reset_control_get_optional_exclusive(dev, "phy");

> +	if (IS_ERR(port->phy_reset))

> +		return PTR_ERR(port->phy_reset);

> +

> +	/* PHY power on and enable pipe clock */

> +	port->phy = devm_phy_optional_get(dev, "pcie-phy");

> +	if (IS_ERR(port->phy))

> +		return PTR_ERR(port->phy);

> +

> +	reset_control_deassert(port->phy_reset);

> +

> +	err = phy_power_on(port->phy);

> +	if (err) {

> +		dev_notice(dev, "failed to power on pcie phy\n");

> +		goto err_phy_on;

> +	}

> +

> +	err = phy_init(port->phy);

> +	if (err) {

> +		dev_notice(dev, "failed to initialize pcie phy\n");

> +		goto err_phy_init;

> +	}

> +

> +	port->mac_reset = devm_reset_control_get_optional_exclusive(dev, "mac");

> +	if (IS_ERR(port->mac_reset)) {

> +		err = PTR_ERR(port->mac_reset);

> +		goto err_mac_rst;

> +	}

> +

> +	reset_control_deassert(port->mac_reset);

> +

> +	/* MAC power on and enable transaction layer clocks */

> +	pm_runtime_enable(dev);

> +	pm_runtime_get_sync(dev);

> +

> +	err = mtk_pcie_clk_init(port);

> +	if (err) {

> +		dev_notice(dev, "clock init failed\n");

> +		goto err_clk_init;

> +	}

> +

> +	return 0;

> +

> +err_clk_init:

> +	pm_runtime_put_sync(dev);

> +	pm_runtime_disable(dev);

> +	reset_control_assert(port->mac_reset);

> +err_mac_rst:

> +	phy_exit(port->phy);

> +err_phy_init:

> +	phy_power_off(port->phy);

> +err_phy_on:

> +	reset_control_assert(port->phy_reset);

> +

> +	return err;

> +}

> +

> +static void mtk_pcie_power_down(struct mtk_pcie_port *port)

> +{

> +	clk_bulk_disable_unprepare(port->num_clks, port->clks);

> +

> +	pm_runtime_put_sync(port->dev);

> +	pm_runtime_disable(port->dev);

> +	reset_control_assert(port->mac_reset);

> +

> +	phy_power_off(port->phy);

> +	phy_exit(port->phy);

> +	reset_control_assert(port->phy_reset);

> +}

> +

> +static int mtk_pcie_setup(struct mtk_pcie_port *port)

> +{

> +	struct device *dev = port->dev;

> +	struct platform_device *pdev = to_platform_device(dev);

> +	struct resource *regs;

> +	int err;

> +

> +	regs = platform_get_resource_byname(pdev, IORESOURCE_MEM, "pcie-mac");

> +	port->base = devm_ioremap_resource(dev, regs);

> +	if (IS_ERR(port->base)) {

> +		dev_notice(dev, "failed to map register base\n");

> +		return PTR_ERR(port->base);

> +	}

> +

> +	port->reg_base = regs->start;

> +

> +	/* Don't touch the hardware registers before power up */

> +	err = mtk_pcie_power_up(port);

> +	if (err)

> +		return err;

> +

> +	/* Try link up */

> +	err = mtk_pcie_startup_port(port);

> +	if (err) {

> +		dev_notice(dev, "PCIe link down\n");

> +		goto err_setup;

> +	}

> +

> +	err = mtk_pcie_setup_irq(port, dev->of_node);

> +	if (err)

> +		goto err_setup;

> +

> +	dev_info(dev, "PCIe link up success!\n");

> +

> +	return 0;

> +

> +err_setup:

> +	mtk_pcie_power_down(port);

> +

> +	return err;

> +}

> +

> +static int mtk_pcie_probe(struct platform_device *pdev)

> +{

> +	struct device *dev = &pdev->dev;

> +	struct mtk_pcie_port *port;

> +	struct pci_host_bridge *host;

> +	int err;

> +

> +	host = devm_pci_alloc_host_bridge(dev, sizeof(*port));

> +	if (!host)

> +		return -ENOMEM;

> +

> +	port = pci_host_bridge_priv(host);

> +

> +	port->dev = dev;

> +	platform_set_drvdata(pdev, port);

> +

> +	err = mtk_pcie_setup(port);

> +	if (err)

> +		return err;

> +

> +	host->ops = &mtk_pcie_ops;

> +	host->sysdata = port;

> +

> +	err = pci_host_probe(host);

> +	if (err) {

> +		mtk_pcie_power_down(port);


mtk_pcie_irq_teardown needed here?

> +		return err;

> +	}

> +

> +	return 0;

> +}

> +

> +static int mtk_pcie_remove(struct platform_device *pdev)

> +{

> +	struct mtk_pcie_port *port = platform_get_drvdata(pdev);

> +	struct pci_host_bridge *host = pci_host_bridge_from_priv(port);

> +

> +	pci_lock_rescan_remove();

> +	pci_stop_root_bus(host->bus);

> +	pci_remove_root_bus(host->bus);

> +	pci_unlock_rescan_remove();

> +

> +	mtk_pcie_irq_teardown(port);

> +	mtk_pcie_power_down(port);

> +

> +	return 0;

> +}

> +

> +static int __maybe_unused mtk_pcie_turn_off_link(struct mtk_pcie_port *port)

> +{

> +	u32 val;

> +

> +	val = readl(port->base + PCIE_ICMD_PM_REG);

> +	val |= PCIE_TURN_OFF_LINK;

> +	writel(val, port->base + PCIE_ICMD_PM_REG);

> +

> +	/* Check the link is L2 */

> +	return readl_poll_timeout(port->base + PCIE_LTSSM_STATUS_REG, val,

> +				  (PCIE_LTSSM_STATE(val) ==

> +				   PCIE_LTSSM_STATE_L2_IDLE), 20,

> +				   50 * USEC_PER_MSEC);

> +}

> +

> +static int __maybe_unused mtk_pcie_suspend_noirq(struct device *dev)

> +{

> +	struct mtk_pcie_port *port = dev_get_drvdata(dev);

> +	int err;

> +	u32 val;

> +

> +	if (port->is_suspended)

> +		return 0;


I don't think this variable is needed. The core should track calling 
suspend/resume.

> +

> +	/* Trigger link to L2 state */

> +	err = mtk_pcie_turn_off_link(port);

> +	if (err) {

> +		dev_notice(port->dev, "can not enter L2 state\n");


Seems more like an error.

> +		goto power_off;

> +	}

> +

> +	/* Pull down the PERST# pin */

> +	val = readl(port->base + PCIE_RST_CTRL_REG);

> +	val |= PCIE_PE_RSTB;

> +	writel(val, port->base + PCIE_RST_CTRL_REG);

> +

> +	dev_info(port->dev, "enter L2 state success");


Seems like debug.

> +

> +power_off:

> +	clk_bulk_disable_unprepare(port->num_clks, port->clks);

> +

> +	phy_power_off(port->phy);

> +

> +	port->is_suspended = true;

> +

> +	return 0;

> +}

> +

> +static int __maybe_unused mtk_pcie_resume_noirq(struct device *dev)

> +{

> +	struct mtk_pcie_port *port = dev_get_drvdata(dev);

> +	int err;

> +

> +	if (!port->is_suspended)

> +		return 0;

> +

> +	phy_power_on(port->phy);

> +

> +	err = clk_bulk_prepare_enable(port->num_clks, port->clks);

> +	if (err) {

> +		dev_dbg(dev, "failed to enable pcie clocks\n");

> +		return err;

> +	}

> +

> +	err = mtk_pcie_startup_port(port);

> +	if (err) {

> +		dev_notice(port->dev, "resume failed\n");

> +		return err;

> +	}

> +

> +	port->is_suspended = false;

> +

> +	dev_info(port->dev, "resume done\n");


Debug...

> +

> +	return 0;

> +}

> +

> +static const struct dev_pm_ops mtk_pcie_pm_ops = {

> +	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(mtk_pcie_suspend_noirq,

> +				      mtk_pcie_resume_noirq)

> +};

> +

> +static const struct of_device_id mtk_pcie_of_match[] = {

> +	{ .compatible = "mediatek,mt8192-pcie" },

> +	{},

> +};

> +

> +static struct platform_driver mtk_pcie_driver = {

> +	.probe = mtk_pcie_probe,

> +	.remove = mtk_pcie_remove,

> +	.driver = {

> +		.name = "mtk-pcie",

> +		.of_match_table = mtk_pcie_of_match,

> +		.pm = &mtk_pcie_pm_ops,

> +	},

> +};

> +

> +module_platform_driver(mtk_pcie_driver);

> +MODULE_LICENSE("GPL v2");

> -- 

> 2.25.1

> _______________________________________________

> linux-arm-kernel mailing list

> linux-arm-kernel@lists.infradead.org

> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

"Add new generation" really contains no information.  And "mediatek"
is already used for the pcie-mediatek.c driver, so we should have a
new tag for this new driver.  Include useful information in the
subject, e.g.,

  PCI: mediatek-gen3: Add MediaTek Gen3 driver for MT8192

On Wed, Nov 18, 2020 at 04:29:34PM +0800, Jianjun Wang wrote:
> MediaTek's PCIe host controller has three generation HWs, the new

> generation HW is an individual bridge, it supoorts Gen3 speed and

> up to 256 MSI interrupt numbers for multi-function devices.


s/supoorts/supports/

> Add support for new Gen3 controller which can be found on MT8192.

> 

> Signed-off-by: Jianjun Wang <jianjun.wang@mediatek.com>

> Acked-by: Ryder Lee <ryder.lee@mediatek.com>

> ---

>  drivers/pci/controller/Kconfig              |   14 +

>  drivers/pci/controller/Makefile             |    1 +

>  drivers/pci/controller/pcie-mediatek-gen3.c | 1031 +++++++++++++++++++

>  3 files changed, 1046 insertions(+)

>  create mode 100644 drivers/pci/controller/pcie-mediatek-gen3.c

> 

> diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig

> index f18c3725ef80..83daa772595b 100644

> --- a/drivers/pci/controller/Kconfig

> +++ b/drivers/pci/controller/Kconfig

> @@ -239,6 +239,20 @@ config PCIE_MEDIATEK

>  	  Say Y here if you want to enable PCIe controller support on

>  	  MediaTek SoCs.

>  

> +config PCIE_MEDIATEK_GEN3

> +	tristate "MediaTek GEN3 PCIe controller"


s/GEN3/Gen3/

> +	depends on ARCH_MEDIATEK || COMPILE_TEST

> +	depends on OF


Do you really need "depends on OF"?  There are stubs to cover most
interfaces even when CONFIG_OF is not set.  Depending on OF reduces
compile-test coverage.

> +	depends on PCI_MSI_IRQ_DOMAIN

> +	help

> +	  Adds support for PCIe Gen3 MAC controller for MediaTek SoCs.

> +	  This PCIe controller provides the capable of Gen3, Gen2 and


"provides the capable of" doesn't parse.

> +	  Gen1 speed, and support up to 256 MSI interrupt numbers for

> +	  multi-function devices.

> +

> +	  Say Y here if you want to enable Gen3 PCIe controller support on

> +	  MediaTek SoCs.

> +

>  config PCIE_TANGO_SMP8759

>  	bool "Tango SMP8759 PCIe controller (DANGEROUS)"

>  	depends on ARCH_TANGO && PCI_MSI && OF

> diff --git a/drivers/pci/controller/Makefile b/drivers/pci/controller/Makefile

> index bcdbf49ab1e4..9c1b96777597 100644

> --- a/drivers/pci/controller/Makefile

> +++ b/drivers/pci/controller/Makefile

> @@ -27,6 +27,7 @@ obj-$(CONFIG_PCIE_ROCKCHIP) += pcie-rockchip.o

>  obj-$(CONFIG_PCIE_ROCKCHIP_EP) += pcie-rockchip-ep.o

>  obj-$(CONFIG_PCIE_ROCKCHIP_HOST) += pcie-rockchip-host.o

>  obj-$(CONFIG_PCIE_MEDIATEK) += pcie-mediatek.o

> +obj-$(CONFIG_PCIE_MEDIATEK_GEN3) += pcie-mediatek-gen3.o

>  obj-$(CONFIG_PCIE_TANGO_SMP8759) += pcie-tango.o

>  obj-$(CONFIG_VMD) += vmd.o

>  obj-$(CONFIG_PCIE_BRCMSTB) += pcie-brcmstb.o

> diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c b/drivers/pci/controller/pcie-mediatek-gen3.c

> new file mode 100644

> index 000000000000..dd454cfee373

> --- /dev/null

> +++ b/drivers/pci/controller/pcie-mediatek-gen3.c

> @@ -0,0 +1,1031 @@

> +// SPDX-License-Identifier: GPL-2.0

> +/*

> + * MediaTek PCIe host controller driver.

> + *

> + * Copyright (c) 2020 MediaTek Inc.

> + * Author: Jianjun Wang <jianjun.wang@mediatek.com>

> + */

> +

> +#include <linux/clk.h>

> +#include <linux/delay.h>

> +#include <linux/iopoll.h>

> +#include <linux/irq.h>

> +#include <linux/irqchip/chained_irq.h>

> +#include <linux/irqdomain.h>

> +#include <linux/kernel.h>

> +#include <linux/module.h>

> +#include <linux/msi.h>

> +#include <linux/of_address.h>

> +#include <linux/of_clk.h>

> +#include <linux/of_pci.h>

> +#include <linux/of_platform.h>

> +#include <linux/pci.h>

> +#include <linux/phy/phy.h>

> +#include <linux/platform_device.h>

> +#include <linux/pm_domain.h>

> +#include <linux/pm_runtime.h>

> +#include <linux/reset.h>

> +

> +#include "../pci.h"

> +

> +#define PCIE_SETTING_REG		0x80

> +#define PCIE_PCI_IDS_1			0x9c

> +#define PCI_CLASS(class)		(class << 8)

> +#define PCIE_RC_MODE			BIT(0)

> +

> +#define PCIE_CFGNUM_REG			0x140

> +#define PCIE_CFG_DEVFN(devfn)		((devfn) & GENMASK(7, 0))

> +#define PCIE_CFG_BUS(bus)		(((bus) << 8) & GENMASK(15, 8))

> +#define PCIE_CFG_BYTE_EN(bytes)		(((bytes) << 16) & GENMASK(19, 16))

> +#define PCIE_CFG_FORCE_BYTE_EN		BIT(20)

> +#define PCIE_CFG_OFFSET_ADDR		0x1000

> +#define PCIE_CFG_HEADER(devfn, bus) \

> +	(PCIE_CFG_DEVFN(devfn) | PCIE_CFG_BUS(bus))


This should be:

  (PCIE_CFG_BUS(bus) | PCIE_CFG_DEVFN(devfn))

to match the order of the bits in the address you're constructing.

> +#define PCIE_CFG_HEADER_FORCE_BE(devfn, bus, bytes) \

> +	(PCIE_CFG_HEADER(devfn, bus) | PCIE_CFG_BYTE_EN(bytes) \

> +	 | PCIE_CFG_FORCE_BYTE_EN)


Reorder similarly.

> +#define PCIE_RST_CTRL_REG		0x148

> +#define PCIE_MAC_RSTB			BIT(0)

> +#define PCIE_PHY_RSTB			BIT(1)

> +#define PCIE_BRG_RSTB			BIT(2)

> +#define PCIE_PE_RSTB			BIT(3)

> +

> +#define PCIE_MISC_STATUS_REG		0x14C

> +#define PCIE_LTR_MSG_RECEIVED		BIT(0)

> +#define PCIE_PCIE_MSG_RECEIVED		BIT(1)


Unused, please remove.

> +#define PCIE_LTSSM_STATUS_REG		0x150

> +#define PCIE_LTSSM_STATE_MASK		GENMASK(28, 24)

> +#define PCIE_LTSSM_STATE(val)		((val & PCIE_LTSSM_STATE_MASK) >> 24)


> +#define PCIE_LTSSM_STATE_L0		0x10

> +#define PCIE_LTSSM_STATE_L1_IDLE	0x13


Unused, please remove.

> +#define PCIE_LTSSM_STATE_L2_IDLE	0x14

> +

> +#define PCIE_LINK_STATUS_REG		0x154

> +#define PCIE_PORT_LINKUP		BIT(8)

> +

> +#define PCIE_MSI_SET_NUM		8

> +#define PCIE_MSI_IRQS_PER_SET		32

> +#define PCIE_MSI_IRQS_NUM \

> +	(PCIE_MSI_IRQS_PER_SET * (PCIE_MSI_SET_NUM))

> +

> +#define PCIE_INT_ENABLE_REG		0x180

> +#define PCIE_MSI_MASK			GENMASK(PCIE_MSI_SET_NUM + 8 - 1, 8)

> +#define PCIE_MSI_SHIFT			8

> +#define PCIE_INTX_SHIFT			24

> +#define PCIE_INTX_MASK			GENMASK(27, 24)


> +#define PCIE_MSG_MASK			BIT(28)

> +#define PCIE_AER_MASK			BIT(29)

> +#define PCIE_PM_MASK			BIT(30)


Unused, please remove.

> +#define PCIE_INT_STATUS_REG		0x184

> +#define PCIE_MSI_SET_ENABLE_REG		0x190

> +

> +#define PCIE_LOW_POWER_CTRL_REG		0x194

> +#define PCIE_DIS_LOWPWR_MASK		GENMASK(3, 0)

> +#define PCIE_DIS_L0S_MASK		BIT(0)

> +#define PCIE_DIS_L1_MASK		BIT(1)

> +#define PCIE_DIS_L11_MASK		BIT(2)

> +#define PCIE_DIS_L12_MASK		BIT(3)

> +#define PCIE_FORCE_DIS_LOWPWR		GENMASK(11, 8)

> +#define PCIE_FORCE_DIS_L0S		BIT(8)

> +#define PCIE_FORCE_DIS_L1		BIT(9)

> +#define PCIE_FORCE_DIS_L11		BIT(10)

> +#define PCIE_FORCE_DIS_L12		BIT(11)


Unused, please remove.

> +#define PCIE_ICMD_PM_REG		0x198

> +#define PCIE_TURN_OFF_LINK		BIT(4)

> +

> +#define PCIE_MSI_ADDR_BASE_REG		0xc00

> +#define PCIE_MSI_SET_OFFSET		0x10

> +#define PCIE_MSI_STATUS_OFFSET		0x04

> +#define PCIE_MSI_ENABLE_OFFSET		0x08

> +

> +#define PCIE_TRANS_TABLE_BASE_REG	0x800

> +#define PCIE_ATR_SRC_ADDR_MSB_OFFSET	0x4

> +#define PCIE_ATR_TRSL_ADDR_LSB_OFFSET	0x8

> +#define PCIE_ATR_TRSL_ADDR_MSB_OFFSET	0xc

> +#define PCIE_ATR_TRSL_PARAM_OFFSET	0x10

> +#define PCIE_ATR_TLB_SET_OFFSET		0x20

> +

> +#define PCIE_MAX_TRANS_TABLES		8

> +#define ATR_EN				BIT(0)

> +#define ATR_SIZE(size)			((((size) - 1) << 1) & GENMASK(6, 1))

> +#define ATR_ID(id)			(id & GENMASK(3, 0))

> +#define ATR_PARAM(param)		(((param) << 16) & GENMASK(27, 16))

> +

> +/**

> + * struct mtk_pcie_msi - MSI information for each set

> + * @base: IO mapped register base

> + * @irq: MSI set Interrupt number

> + * @index: MSI set number

> + * @msg_addr: MSI message address

> + * @domain: IRQ domain

> + */

> +struct mtk_pcie_msi {

> +	void __iomem *base;

> +	unsigned int irq;

> +	int index;

> +	phys_addr_t msg_addr;

> +	struct irq_domain *domain;

> +};

> +

> +/**

> + * struct mtk_pcie_port - PCIe port information

> + * @dev: PCIe device

> + * @base: IO mapped register base

> + * @reg_base: Physical register base

> + * @mac_reset: mac reset control

> + * @phy_reset: phy reset control

> + * @phy: PHY controller block

> + * @clks: PCIe clocks

> + * @num_clks: PCIe clocks count for this port

> + * @is_suspended: device suspend state

> + * @irq: PCIe controller interrupt number

> + * @intx_domain: legacy INTx IRQ domain

> + * @msi_domain: MSI IRQ domain

> + * @msi_top_domain: MSI IRQ top domain

> + * @msi_info: MSI sets information

> + * @lock: lock protecting IRQ bit map

> + * @msi_irq_in_use: bit map for assigned MSI IRQ

> + */

> +struct mtk_pcie_port {

> +	struct device *dev;

> +	void __iomem *base;

> +	phys_addr_t reg_base;

> +	struct reset_control *mac_reset;

> +	struct reset_control *phy_reset;

> +	struct phy *phy;

> +	struct clk_bulk_data *clks;

> +	int num_clks;

> +	bool is_suspended;

> +

> +	int irq;

> +	struct irq_domain *intx_domain;

> +	struct irq_domain *msi_domain;

> +	struct irq_domain *msi_top_domain;

> +	struct mtk_pcie_msi **msi_info;

> +	struct mutex lock;

> +	DECLARE_BITMAP(msi_irq_in_use, PCIE_MSI_IRQS_NUM);

> +};

> +

> +static int mtk_pcie_config_read(struct pci_bus *bus, unsigned int devfn,

> +				    int where, int size, u32 *val)

> +{

> +	struct mtk_pcie_port *port = bus->sysdata;

> +	int bytes;

> +

> +	bytes = ((1 << size) - 1) << (where & 0x3);


This seems like some unusual bit twiddling; at least, I don't remember
seeing this before.  Can you skim other drivers and see if others do
the same thing, and adopt a common style if they do?

> +	writel(PCIE_CFG_HEADER_FORCE_BE(devfn, bus->number, bytes),

> +	       port->base + PCIE_CFGNUM_REG);

> +

> +	*val = readl(port->base + PCIE_CFG_OFFSET_ADDR + (where & ~0x3));


These look like they need to be atomic, since you need a writel()
followed by a readl().

pci_lock_config() (used in pci_bus_read_config_*(), etc) uses the
global pci_lock for this unless CONFIG_PCI_LOCKLESS_CONFIG is set.

But I would like to eventually move away from this implicit dependency
on pci_lock.  If you need to make this atomic, can you add the
explicit locking here, so there's a clear connection between the lock
and the things it protects?

> +	if (size <= 2)

> +		*val = (*val >> (8 * (where & 0x3))) & ((1 << (size * 8)) - 1);

> +

> +	return PCIBIOS_SUCCESSFUL;

> +}

> +

> +static int mtk_pcie_config_write(struct pci_bus *bus, unsigned int devfn,

> +				     int where, int size, u32 val)

> +{

> +	struct mtk_pcie_port *port = bus->sysdata;

> +	int bytes;

> +

> +	bytes = ((1 << size) - 1) << (where & 0x3);

> +	writel(PCIE_CFG_HEADER_FORCE_BE(devfn, bus->number, bytes),

> +	       port->base + PCIE_CFGNUM_REG);

> +

> +	if (size <= 2)

> +		val = (val & ((1 << (size * 8)) - 1)) << ((where & 0x3) * 8);

> +

> +	writel(val, port->base + PCIE_CFG_OFFSET_ADDR + (where & ~0x3));

> +

> +	return PCIBIOS_SUCCESSFUL;

> +}

> +

> +static struct pci_ops mtk_pcie_ops = {

> +	.read  = mtk_pcie_config_read,

> +	.write = mtk_pcie_config_write,

> +};

> +

> +static void mtk_pcie_set_trans_window(void __iomem *reg,

> +				      resource_size_t cpu_addr,

> +				      resource_size_t pci_addr, size_t size)

> +{

> +	writel(lower_32_bits(cpu_addr) | ATR_SIZE(fls(size) - 1) | ATR_EN, reg);

> +	writel(upper_32_bits(cpu_addr), reg + PCIE_ATR_SRC_ADDR_MSB_OFFSET);

> +	writel(lower_32_bits(pci_addr), reg + PCIE_ATR_TRSL_ADDR_LSB_OFFSET);

> +	writel(upper_32_bits(pci_addr), reg + PCIE_ATR_TRSL_ADDR_MSB_OFFSET);

> +	writel(ATR_ID(0) | ATR_PARAM(0), reg + PCIE_ATR_TRSL_PARAM_OFFSET);

> +}

> +

> +static int mtk_pcie_set_trans_table(void __iomem *reg,

> +				    resource_size_t cpu_addr,

> +				    resource_size_t pci_addr, size_t size,

> +				    int num)

> +{

> +	void __iomem *table_base;

> +

> +	if (num > PCIE_MAX_TRANS_TABLES)

> +		return -ENODEV;

> +

> +	table_base = reg + num * PCIE_ATR_TLB_SET_OFFSET;

> +	mtk_pcie_set_trans_window(table_base, cpu_addr, pci_addr, size);

> +

> +	return 0;

> +}

> +

> +static int mtk_pcie_startup_port(struct mtk_pcie_port *port)

> +{

> +	struct resource_entry *entry;

> +	struct pci_host_bridge *host = pci_host_bridge_from_priv(port);

> +	unsigned int table_index = 0;

> +	int err;

> +	u32 val;

> +

> +	/* Set as RC mode */

> +	val = readl(port->base + PCIE_SETTING_REG);

> +	val |= PCIE_RC_MODE;

> +	writel(val, port->base + PCIE_SETTING_REG);

> +

> +	/* Set class code */

> +	val = readl(port->base + PCIE_PCI_IDS_1);

> +	val &= ~GENMASK(31, 8);

> +	val |= PCI_CLASS(PCI_CLASS_BRIDGE_PCI << 8);

> +	writel(val, port->base + PCIE_PCI_IDS_1);

> +

> +	/* Assert all reset signals */

> +	val = readl(port->base + PCIE_RST_CTRL_REG);

> +	val |= PCIE_MAC_RSTB | PCIE_PHY_RSTB | PCIE_BRG_RSTB | PCIE_PE_RSTB;

> +	writel(val, port->base + PCIE_RST_CTRL_REG);

> +

> +	/* De-assert reset signals*/


Missing space at end of comment.

> +	val &= ~(PCIE_MAC_RSTB | PCIE_PHY_RSTB | PCIE_BRG_RSTB);

> +	writel(val, port->base + PCIE_RST_CTRL_REG);

> +

> +	/* Delay 100ms to wait the reference clocks become stable */

> +	usleep_range(100 * 1000, 120 * 1000);

> +

> +	/* De-assert pe reset*/


Missing space at end of comment.  "pe" looks like something that
should be capitalized, since I assume it's some sort of initialism.

> +	val &= ~PCIE_PE_RSTB;

> +	writel(val, port->base + PCIE_RST_CTRL_REG);

> +

> +	/* Check if the link is up or not */

> +	err = readl_poll_timeout(port->base + PCIE_LINK_STATUS_REG, val,

> +			!!(val & PCIE_PORT_LINKUP), 20,

> +			50 * USEC_PER_MSEC);

> +	if (err) {

> +		val = readl(port->base + PCIE_LTSSM_STATUS_REG);

> +		dev_dbg(port->dev, "ltssm reg val: %#x\n", val);

> +		return err;

> +	}

> +

> +	/* Set PCIe translation windows */

> +	resource_list_for_each_entry(entry, &host->windows) {

> +		unsigned long type = resource_type(entry->res);

> +		struct resource *res = NULL;

> +		resource_size_t cpu_addr;

> +		resource_size_t pci_addr;

> +

> +		if (!(type & (IORESOURCE_MEM | IORESOURCE_IO)))

> +			continue;

> +

> +		res = entry->res;

> +		cpu_addr = res->start;

> +		pci_addr = res->start - entry->offset;

> +		mtk_pcie_set_trans_table(port->base + PCIE_TRANS_TABLE_BASE_REG,

> +					 cpu_addr, pci_addr, resource_size(res),

> +					 table_index);

> +

> +		dev_dbg(port->dev, "Set %s trans window[%d]: cpu_addr = %#llx, pci_addr = %#llx, size = %#llx\n",

> +			(!!(type & IORESOURCE_MEM) ? "MEM" : "IO"), table_index,

> +			cpu_addr, pci_addr, resource_size(res));

> +

> +		table_index++;

> +	}

> +

> +	return 0;

> +}

> +

> +static inline struct mtk_pcie_msi *mtk_get_msi_info(struct mtk_pcie_port *port,

> +						    unsigned long hwirq)

> +{

> +	return port->msi_info[hwirq / PCIE_MSI_IRQS_PER_SET];

> +}

> +

> +static int mtk_pcie_set_affinity(struct irq_data *data,

> +				 const struct cpumask *mask, bool force)

> +{

> +	struct mtk_pcie_port *port = irq_data_get_irq_chip_data(data);

> +	int ret;

> +

> +	ret = irq_set_affinity_hint(port->irq, mask);

> +	if (ret)

> +		return ret;

> +

> +	irq_data_update_effective_affinity(data, mask);

> +

> +	return 0;

> +}

> +

> +static void mtk_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)

> +{

> +	struct mtk_pcie_msi *msi_info;

> +	struct mtk_pcie_port *port = irq_data_get_irq_chip_data(data);

> +

> +	msi_info = mtk_get_msi_info(port, data->hwirq);

> +

> +	msg->address_hi = 0;

> +	msg->address_lo = lower_32_bits(msi_info->msg_addr);

> +

> +	msg->data = data->hwirq;

> +	dev_dbg(port->dev, "msi#%#lx address_hi %#x address_lo %#x data %d\n",

> +		data->hwirq, msg->address_hi, msg->address_lo, msg->data);

> +}

> +

> +static void mtk_msi_irq_ack(struct irq_data *data)

> +{

> +	struct mtk_pcie_msi *msi_info;

> +	struct mtk_pcie_port *port = irq_data_get_irq_chip_data(data);

> +	unsigned long hwirq;

> +

> +	msi_info = mtk_get_msi_info(port, data->hwirq);

> +

> +	hwirq =	data->hwirq % PCIE_MSI_IRQS_PER_SET;

> +

> +	writel(1 << hwirq, msi_info->base + PCIE_MSI_STATUS_OFFSET);

> +}

> +

> +static void mtk_msi_irq_mask(struct irq_data *data)

> +{

> +	struct mtk_pcie_msi *msi_info;

> +	struct mtk_pcie_port *port = irq_data_get_irq_chip_data(data);

> +	u32 val;

> +

> +	msi_info = mtk_get_msi_info(port, data->hwirq);

> +	val = readl(msi_info->base + PCIE_MSI_ENABLE_OFFSET);

> +	val &= ~(1 << data->hwirq);

> +	writel(val, msi_info->base + PCIE_MSI_ENABLE_OFFSET);

> +

> +	pci_msi_mask_irq(data);

> +}

> +

> +static void mtk_msi_irq_unmask(struct irq_data *data)

> +{

> +	struct mtk_pcie_msi *msi_info;

> +	struct mtk_pcie_port *port = irq_data_get_irq_chip_data(data);

> +	u32 val;

> +

> +	msi_info = mtk_get_msi_info(port, data->hwirq);

> +	val = readl(msi_info->base + PCIE_MSI_ENABLE_OFFSET);

> +	val |= (1 << data->hwirq);

> +	writel(val, msi_info->base + PCIE_MSI_ENABLE_OFFSET);

> +

> +	pci_msi_unmask_irq(data);

> +}

> +

> +static struct irq_chip mtk_msi_irq_chip = {

> +	.irq_ack		= mtk_msi_irq_ack,

> +	.irq_compose_msi_msg	= mtk_compose_msi_msg,

> +	.irq_mask		= mtk_msi_irq_mask,

> +	.irq_unmask		= mtk_msi_irq_unmask,

> +	.irq_set_affinity	= mtk_pcie_set_affinity,

> +	.name			= "PCIe",

> +};

> +

> +static irq_hw_number_t mtk_pcie_msi_get_hwirq(struct msi_domain_info *info,

> +					      msi_alloc_info_t *arg)

> +{

> +	struct msi_desc *entry = arg->desc;

> +	struct mtk_pcie_port *port = info->chip_data;

> +	int hwirq;

> +

> +	mutex_lock(&port->lock);

> +

> +	hwirq = bitmap_find_free_region(port->msi_irq_in_use, PCIE_MSI_IRQS_NUM,

> +			order_base_2(entry->nvec_used));

> +	if (hwirq < 0) {

> +		mutex_unlock(&port->lock);

> +		return -ENOSPC;

> +	}

> +

> +	mutex_unlock(&port->lock);

> +

> +	return hwirq;

> +}

> +

> +static void mtk_pcie_msi_free(struct irq_domain *domain,

> +			      struct msi_domain_info *info, unsigned int virq)

> +{

> +	struct irq_data *data = irq_domain_get_irq_data(domain, virq);

> +	struct mtk_pcie_port *port = irq_data_get_irq_chip_data(data);

> +

> +	mutex_lock(&port->lock);

> +

> +	bitmap_clear(port->msi_irq_in_use, data->hwirq, 1);

> +

> +	mutex_unlock(&port->lock);

> +}

> +

> +static struct msi_domain_ops mtk_msi_domain_ops = {

> +	.get_hwirq	= mtk_pcie_msi_get_hwirq,

> +	.msi_free	= mtk_pcie_msi_free,

> +};

> +

> +static struct msi_domain_info mtk_msi_domain_info = {

> +	.flags		= (MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_PCI_MSIX |

> +			   MSI_FLAG_USE_DEF_CHIP_OPS | MSI_FLAG_MULTI_PCI_MSI),

> +	.chip		= &mtk_msi_irq_chip,

> +	.ops		= &mtk_msi_domain_ops,

> +	.handler	= handle_edge_irq,

> +	.handler_name	= "MSI",

> +};

> +

> +static void mtk_msi_top_irq_eoi(struct irq_data *data)

> +{

> +	struct mtk_pcie_port *port = irq_data_get_irq_chip_data(data);

> +	unsigned long msi_irq = data->hwirq + PCIE_MSI_SHIFT;

> +

> +	writel(1 << msi_irq, port->base + PCIE_INT_STATUS_REG);

> +}

> +

> +static struct irq_chip mtk_msi_top_irq_chip = {

> +	.irq_eoi	= mtk_msi_top_irq_eoi,

> +	.name		= "PCIe",

> +};

> +

> +static void mtk_pcie_msi_handler(struct irq_desc *desc)

> +{

> +	struct mtk_pcie_msi *msi_info = irq_desc_get_handler_data(desc);

> +	struct irq_chip *irqchip = irq_desc_get_chip(desc);

> +	unsigned long msi_enable, msi_status;

> +	unsigned int virq;

> +	irq_hw_number_t bit, hwirq;

> +

> +	chained_irq_enter(irqchip, desc);

> +

> +	msi_enable = readl(msi_info->base + PCIE_MSI_ENABLE_OFFSET);

> +	while ((msi_status = readl(msi_info->base + PCIE_MSI_STATUS_OFFSET))) {

> +		msi_status &= msi_enable;

> +		for_each_set_bit(bit, &msi_status, PCIE_MSI_IRQS_PER_SET) {

> +			hwirq = bit + msi_info->index * PCIE_MSI_IRQS_PER_SET;

> +			virq = irq_find_mapping(msi_info->domain, hwirq);

> +			generic_handle_irq(virq);

> +		}

> +	}

> +

> +	chained_irq_exit(irqchip, desc);

> +}

> +

> +static int mtk_msi_top_domain_map(struct irq_domain *domain,

> +				    unsigned int virq, irq_hw_number_t hwirq)

> +{

> +	struct mtk_pcie_port *port = domain->host_data;

> +	struct mtk_pcie_msi *msi_info = port->msi_info[hwirq];

> +

> +	irq_domain_set_info(domain, virq, hwirq,

> +			    &mtk_msi_top_irq_chip, domain->host_data,

> +			    mtk_pcie_msi_handler, msi_info, NULL);

> +

> +	return 0;

> +}

> +

> +static const struct irq_domain_ops mtk_msi_top_domain_ops = {

> +	.map = mtk_msi_top_domain_map,

> +};

> +

> +static void mtk_intx_mask(struct irq_data *data)

> +{

> +	struct mtk_pcie_port *port = irq_data_get_irq_chip_data(data);

> +	u32 val;

> +

> +	val = readl(port->base + PCIE_INT_ENABLE_REG);

> +	val &= ~(1 << (data->hwirq + PCIE_INTX_SHIFT));

> +	writel(val, port->base + PCIE_INT_ENABLE_REG);

> +}

> +

> +static void mtk_intx_unmask(struct irq_data *data)

> +{

> +	struct mtk_pcie_port *port = irq_data_get_irq_chip_data(data);

> +	u32 val;

> +

> +	val = readl(port->base + PCIE_INT_ENABLE_REG);

> +	val |= 1 << (data->hwirq + PCIE_INTX_SHIFT);

> +	writel(val, port->base + PCIE_INT_ENABLE_REG);

> +}

> +

> +static void mtk_intx_eoi(struct irq_data *data)

> +{

> +	struct mtk_pcie_port *port = irq_data_get_irq_chip_data(data);

> +	unsigned long hwirq;

> +

> +	/**

> +	 * As an emulated level irq, its interrupt status will be remained

> +	 * until receive the corresponding message of de-assert, hence that

> +	 * the status can only be cleared when the interrupt has been serviced.


s/irq/IRQ/

  As an emulated level IRQ, its interrupt status will remain until the
  corresponding de-assert message is received; hence that ...

> +	 */

> +	hwirq = data->hwirq + PCIE_INTX_SHIFT;

> +	writel(1 << hwirq, port->base + PCIE_INT_STATUS_REG);

> +}

> +

> +static struct irq_chip mtk_intx_irq_chip = {

> +	.irq_mask		= mtk_intx_mask,

> +	.irq_unmask		= mtk_intx_unmask,

> +	.irq_eoi		= mtk_intx_eoi,

> +	.irq_set_affinity	= mtk_pcie_set_affinity,

> +	.name			= "PCIe",

> +};

> +

> +static int mtk_pcie_intx_map(struct irq_domain *domain, unsigned int irq,

> +			     irq_hw_number_t hwirq)

> +{

> +	irq_set_chip_and_handler_name(irq, &mtk_intx_irq_chip,

> +				      handle_fasteoi_irq, "INTx");

> +	irq_set_chip_data(irq, domain->host_data);

> +

> +	return 0;

> +}

> +

> +static const struct irq_domain_ops intx_domain_ops = {

> +	.map = mtk_pcie_intx_map,

> +};

> +

> +static int mtk_pcie_init_irq_domains(struct mtk_pcie_port *port,

> +				     struct device_node *node)

> +{

> +	struct device *dev = port->dev;

> +	struct device_node *intc_node;

> +	struct fwnode_handle *fwnode = of_node_to_fwnode(node);

> +	struct mtk_pcie_msi *msi_info;

> +	struct msi_domain_info *info;

> +	int i, ret;

> +

> +	/* Setup INTx */

> +	intc_node = of_get_child_by_name(node, "interrupt-controller");

> +	if (!intc_node) {

> +		dev_notice(dev, "Missing PCIe Intc node\n");


s/Missing/missing/ to match other messages.

> +		return -ENODEV;

> +	}

> +

> +	port->intx_domain = irq_domain_add_linear(intc_node, PCI_NUM_INTX,

> +						  &intx_domain_ops, port);

> +	if (!port->intx_domain) {

> +		dev_notice(dev, "failed to get INTx IRQ domain\n");

> +		return -ENODEV;

> +	}

> +

> +	/* Setup MSI */

> +	mutex_init(&port->lock);

> +

> +	info = devm_kzalloc(dev, sizeof(*info), GFP_KERNEL);

> +	if (!info)

> +		return -ENOMEM;

> +

> +	memcpy(info, &mtk_msi_domain_info, sizeof(*info));

> +	info->chip_data = port;

> +

> +	port->msi_domain = pci_msi_create_irq_domain(fwnode, info, NULL);

> +	if (!port->msi_domain) {

> +		dev_info(dev, "failed to create msi domain\n");


s/msi/MSI/

> +		ret = -ENODEV;

> +		goto err_msi_domain;

> +	}

> +

> +	/* Enable MSI and setup pcie domains */


s/pcie/PCIe/

> +	port->msi_top_domain = irq_domain_add_hierarchy(NULL, 0, 0, node,

> +							&mtk_msi_top_domain_ops,

> +							port);

> +	if (!port->msi_top_domain) {

> +		dev_info(dev, "failed to create msi top domain\n");


s/msi/MSI/

> +		ret = -ENODEV;

> +		goto err_msi_top_domain;

> +	}

> +

> +	port->msi_info = devm_kzalloc(dev, PCIE_MSI_SET_NUM, GFP_KERNEL);

> +	if (!port->msi_info) {

> +		ret = -ENOMEM;

> +		goto err_msi_info;

> +	}

> +

> +	for (i = 0; i < PCIE_MSI_SET_NUM; i++) {

> +		int offset = i * PCIE_MSI_SET_OFFSET;

> +		u32 val;

> +

> +		msi_info = devm_kzalloc(dev, sizeof(*msi_info), GFP_KERNEL);

> +		if (!msi_info) {

> +			ret = -ENOMEM;

> +			goto err_msi_set;

> +		}

> +

> +		msi_info->base = port->base + PCIE_MSI_ADDR_BASE_REG + offset;

> +		msi_info->msg_addr = port->reg_base + PCIE_MSI_ADDR_BASE_REG +

> +				     offset;

> +

> +		writel(lower_32_bits(msi_info->msg_addr), msi_info->base);

> +

> +		msi_info->index = i;

> +		msi_info->domain = port->msi_domain;

> +

> +		port->msi_info[i] = msi_info;

> +

> +		/* Alloc irq for each msi set */


s/irq/IRQ/
s/msi/MSI/

> +		msi_info->irq = irq_create_mapping(port->msi_top_domain, i);

> +		if (!msi_info->irq) {

> +			dev_info(dev, "allocate msi top irq failed\n");


s/irq/IRQ/
s/msi/MSI/

> +			ret = -ENOSPC;

> +			goto err_msi_set;

> +		}

> +

> +		val = readl(port->base + PCIE_INT_ENABLE_REG);

> +		val |= (1 << (i + PCIE_MSI_SHIFT));

> +		writel(val, port->base + PCIE_INT_ENABLE_REG);

> +

> +		val = readl(port->base + PCIE_MSI_SET_ENABLE_REG);

> +		val |= (1 << i);

> +		writel(val, port->base + PCIE_MSI_SET_ENABLE_REG);

> +	}

> +

> +	return 0;

> +

> +err_msi_set:

> +	while (--i >= 0) {


(i-- > 0) would be more idiomatic.

> +		msi_info = port->msi_info[i];

> +		irq_dispose_mapping(msi_info->irq);

> +	}

> +err_msi_info:

> +	irq_domain_remove(port->msi_top_domain);

> +err_msi_top_domain:

> +	irq_domain_remove(port->msi_domain);

> +err_msi_domain:

> +	irq_domain_remove(port->intx_domain);

> +

> +	return ret;

> +}

> +

> +static void mtk_pcie_irq_teardown(struct mtk_pcie_port *port)

> +{

> +	struct mtk_pcie_msi *msi_info;

> +	int i;

> +

> +	irq_set_chained_handler_and_data(port->irq, NULL, NULL);

> +

> +	if (port->intx_domain)

> +		irq_domain_remove(port->intx_domain);

> +

> +	if (port->msi_domain)

> +		irq_domain_remove(port->msi_domain);

> +

> +	if (port->msi_top_domain) {

> +		for (i = 0; i < PCIE_MSI_SET_NUM; i++) {

> +			msi_info = port->msi_info[i];

> +			irq_dispose_mapping(msi_info->irq);

> +		}

> +

> +		irq_domain_remove(port->msi_top_domain);

> +	}

> +

> +	irq_dispose_mapping(port->irq);

> +}

> +

> +static void mtk_pcie_irq_handler(struct irq_desc *desc)

> +{

> +	struct mtk_pcie_port *port = irq_desc_get_handler_data(desc);

> +	struct irq_chip *irqchip = irq_desc_get_chip(desc);

> +	unsigned long status;

> +	unsigned int virq;

> +	irq_hw_number_t irq_bit = PCIE_INTX_SHIFT;

> +

> +	chained_irq_enter(irqchip, desc);

> +

> +	status = readl(port->base + PCIE_INT_STATUS_REG);

> +	if (status & PCIE_INTX_MASK) {

> +		for_each_set_bit_from(irq_bit, &status, PCI_NUM_INTX +

> +				      PCIE_INTX_SHIFT) {

> +			virq = irq_find_mapping(port->intx_domain,

> +						irq_bit - PCIE_INTX_SHIFT);

> +			generic_handle_irq(virq);

> +		}

> +	}

> +

> +	if (status & PCIE_MSI_MASK) {

> +		irq_bit = PCIE_MSI_SHIFT;

> +		for_each_set_bit_from(irq_bit, &status, PCIE_MSI_SET_NUM +

> +				      PCIE_MSI_SHIFT) {

> +			virq = irq_find_mapping(port->msi_top_domain,

> +						irq_bit - PCIE_MSI_SHIFT);

> +			generic_handle_irq(virq);

> +		}

> +	}

> +

> +	chained_irq_exit(irqchip, desc);

> +}

> +

> +static int mtk_pcie_setup_irq(struct mtk_pcie_port *port,

> +			      struct device_node *node)

> +{

> +	struct device *dev = port->dev;

> +	struct platform_device *pdev = to_platform_device(dev);

> +	int err;

> +

> +	err = mtk_pcie_init_irq_domains(port, node);

> +	if (err) {

> +		dev_notice(dev, "failed to init PCIe IRQ domain\n");

> +		return err;

> +	}

> +

> +	port->irq = platform_get_irq(pdev, 0);

> +	if (port->irq < 0)

> +		return port->irq;

> +

> +	irq_set_chained_handler_and_data(port->irq, mtk_pcie_irq_handler, port);

> +

> +	return 0;

> +}

> +

> +static int mtk_pcie_clk_init(struct mtk_pcie_port *port)

> +{

> +	int ret;

> +

> +	port->num_clks = devm_clk_bulk_get_all(port->dev, &port->clks);

> +	if (port->num_clks < 0) {

> +		dev_notice(port->dev, "failed to get pcie clock\n");


s/pcie/PCIe/

> +		return port->num_clks;

> +	}

> +

> +	ret = clk_bulk_prepare_enable(port->num_clks, port->clks);

> +	if (ret) {

> +		dev_notice(port->dev, "failed to enable pcie clocks\n");


s/pcie/PCIe/

> +		return ret;

> +	}

> +

> +	return 0;

> +}

> +

> +static int mtk_pcie_power_up(struct mtk_pcie_port *port)

> +{

> +	struct device *dev = port->dev;

> +	int err;

> +

> +	port->phy_reset = devm_reset_control_get_optional_exclusive(dev, "phy");

> +	if (IS_ERR(port->phy_reset))

> +		return PTR_ERR(port->phy_reset);

> +

> +	/* PHY power on and enable pipe clock */

> +	port->phy = devm_phy_optional_get(dev, "pcie-phy");

> +	if (IS_ERR(port->phy))

> +		return PTR_ERR(port->phy);

> +

> +	reset_control_deassert(port->phy_reset);

> +

> +	err = phy_power_on(port->phy);

> +	if (err) {

> +		dev_notice(dev, "failed to power on pcie phy\n");


s/pcie/PCIe/

> +		goto err_phy_on;

> +	}

> +

> +	err = phy_init(port->phy);

> +	if (err) {

> +		dev_notice(dev, "failed to initialize pcie phy\n");


s/pcie/PCIe/

> +		goto err_phy_init;

> +	}

> +

> +	port->mac_reset = devm_reset_control_get_optional_exclusive(dev, "mac");

> +	if (IS_ERR(port->mac_reset)) {

> +		err = PTR_ERR(port->mac_reset);

> +		goto err_mac_rst;

> +	}

> +

> +	reset_control_deassert(port->mac_reset);

> +

> +	/* MAC power on and enable transaction layer clocks */

> +	pm_runtime_enable(dev);

> +	pm_runtime_get_sync(dev);

> +

> +	err = mtk_pcie_clk_init(port);

> +	if (err) {

> +		dev_notice(dev, "clock init failed\n");

> +		goto err_clk_init;

> +	}

> +

> +	return 0;

> +

> +err_clk_init:

> +	pm_runtime_put_sync(dev);

> +	pm_runtime_disable(dev);

> +	reset_control_assert(port->mac_reset);

> +err_mac_rst:

> +	phy_exit(port->phy);

> +err_phy_init:

> +	phy_power_off(port->phy);

> +err_phy_on:

> +	reset_control_assert(port->phy_reset);

> +

> +	return err;

> +}

> +

> +static void mtk_pcie_power_down(struct mtk_pcie_port *port)

> +{

> +	clk_bulk_disable_unprepare(port->num_clks, port->clks);

> +

> +	pm_runtime_put_sync(port->dev);

> +	pm_runtime_disable(port->dev);

> +	reset_control_assert(port->mac_reset);

> +

> +	phy_power_off(port->phy);

> +	phy_exit(port->phy);

> +	reset_control_assert(port->phy_reset);

> +}

> +

> +static int mtk_pcie_setup(struct mtk_pcie_port *port)

> +{

> +	struct device *dev = port->dev;

> +	struct platform_device *pdev = to_platform_device(dev);

> +	struct resource *regs;

> +	int err;

> +

> +	regs = platform_get_resource_byname(pdev, IORESOURCE_MEM, "pcie-mac");

> +	port->base = devm_ioremap_resource(dev, regs);

> +	if (IS_ERR(port->base)) {

> +		dev_notice(dev, "failed to map register base\n");

> +		return PTR_ERR(port->base);

> +	}

> +

> +	port->reg_base = regs->start;

> +

> +	/* Don't touch the hardware registers before power up */

> +	err = mtk_pcie_power_up(port);

> +	if (err)

> +		return err;

> +

> +	/* Try link up */

> +	err = mtk_pcie_startup_port(port);

> +	if (err) {

> +		dev_notice(dev, "PCIe link down\n");

> +		goto err_setup;


Generally it should not be a fatal error if the link is not up at
probe-time.  You may be able to hot-add a device, or the device may
have some external power control that will power it up later.

> +	}

> +

> +	err = mtk_pcie_setup_irq(port, dev->of_node);

> +	if (err)

> +		goto err_setup;

> +

> +	dev_info(dev, "PCIe link up success!\n");

> +

> +	return 0;

> +

> +err_setup:

> +	mtk_pcie_power_down(port);

> +

> +	return err;

> +}

> +

> +static int mtk_pcie_probe(struct platform_device *pdev)

> +{

> +	struct device *dev = &pdev->dev;

> +	struct mtk_pcie_port *port;

> +	struct pci_host_bridge *host;

> +	int err;

> +

> +	host = devm_pci_alloc_host_bridge(dev, sizeof(*port));

> +	if (!host)

> +		return -ENOMEM;

> +

> +	port = pci_host_bridge_priv(host);

> +

> +	port->dev = dev;

> +	platform_set_drvdata(pdev, port);

> +

> +	err = mtk_pcie_setup(port);

> +	if (err)

> +		return err;

> +

> +	host->ops = &mtk_pcie_ops;

> +	host->sysdata = port;

> +

> +	err = pci_host_probe(host);

> +	if (err) {

> +		mtk_pcie_power_down(port);

> +		return err;

> +	}

> +

> +	return 0;

> +}

> +

> +static int mtk_pcie_remove(struct platform_device *pdev)

> +{

> +	struct mtk_pcie_port *port = platform_get_drvdata(pdev);

> +	struct pci_host_bridge *host = pci_host_bridge_from_priv(port);

> +

> +	pci_lock_rescan_remove();

> +	pci_stop_root_bus(host->bus);

> +	pci_remove_root_bus(host->bus);

> +	pci_unlock_rescan_remove();

> +

> +	mtk_pcie_irq_teardown(port);

> +	mtk_pcie_power_down(port);

> +

> +	return 0;

> +}

> +

> +static int __maybe_unused mtk_pcie_turn_off_link(struct mtk_pcie_port *port)

> +{

> +	u32 val;

> +

> +	val = readl(port->base + PCIE_ICMD_PM_REG);

> +	val |= PCIE_TURN_OFF_LINK;

> +	writel(val, port->base + PCIE_ICMD_PM_REG);

> +

> +	/* Check the link is L2 */

> +	return readl_poll_timeout(port->base + PCIE_LTSSM_STATUS_REG, val,

> +				  (PCIE_LTSSM_STATE(val) ==

> +				   PCIE_LTSSM_STATE_L2_IDLE), 20,

> +				   50 * USEC_PER_MSEC);

> +}

> +

> +static int __maybe_unused mtk_pcie_suspend_noirq(struct device *dev)

> +{

> +	struct mtk_pcie_port *port = dev_get_drvdata(dev);

> +	int err;

> +	u32 val;

> +

> +	if (port->is_suspended)

> +		return 0;

> +

> +	/* Trigger link to L2 state */

> +	err = mtk_pcie_turn_off_link(port);

> +	if (err) {

> +		dev_notice(port->dev, "can not enter L2 state\n");

> +		goto power_off;

> +	}

> +

> +	/* Pull down the PERST# pin */

> +	val = readl(port->base + PCIE_RST_CTRL_REG);

> +	val |= PCIE_PE_RSTB;

> +	writel(val, port->base + PCIE_RST_CTRL_REG);

> +

> +	dev_info(port->dev, "enter L2 state success");

> +

> +power_off:

> +	clk_bulk_disable_unprepare(port->num_clks, port->clks);

> +

> +	phy_power_off(port->phy);

> +

> +	port->is_suspended = true;

> +

> +	return 0;

> +}

> +

> +static int __maybe_unused mtk_pcie_resume_noirq(struct device *dev)

> +{

> +	struct mtk_pcie_port *port = dev_get_drvdata(dev);

> +	int err;

> +

> +	if (!port->is_suspended)

> +		return 0;

> +

> +	phy_power_on(port->phy);

> +

> +	err = clk_bulk_prepare_enable(port->num_clks, port->clks);

> +	if (err) {

> +		dev_dbg(dev, "failed to enable pcie clocks\n");


s/pcie/PCIe/

> +		return err;

> +	}

> +

> +	err = mtk_pcie_startup_port(port);

> +	if (err) {

> +		dev_notice(port->dev, "resume failed\n");

> +		return err;

> +	}

> +

> +	port->is_suspended = false;

> +

> +	dev_info(port->dev, "resume done\n");

> +

> +	return 0;

> +}

> +

> +static const struct dev_pm_ops mtk_pcie_pm_ops = {

> +	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(mtk_pcie_suspend_noirq,

> +				      mtk_pcie_resume_noirq)

> +};

> +

> +static const struct of_device_id mtk_pcie_of_match[] = {

> +	{ .compatible = "mediatek,mt8192-pcie" },

> +	{},

> +};

> +

> +static struct platform_driver mtk_pcie_driver = {

> +	.probe = mtk_pcie_probe,

> +	.remove = mtk_pcie_remove,

> +	.driver = {

> +		.name = "mtk-pcie",

> +		.of_match_table = mtk_pcie_of_match,

> +		.pm = &mtk_pcie_pm_ops,

> +	},

> +};

> +

> +module_platform_driver(mtk_pcie_driver);

> +MODULE_LICENSE("GPL v2");

> -- 

> 2.25.1

>

On Thu, 2020-11-19 at 09:22 -0600, Rob Herring wrote:
> On Wed, Nov 18, 2020 at 04:29:34PM +0800, Jianjun Wang wrote:

> > MediaTek's PCIe host controller has three generation HWs, the new

> > generation HW is an individual bridge, it supoorts Gen3 speed and

> > up to 256 MSI interrupt numbers for multi-function devices.

> > 

> > Add support for new Gen3 controller which can be found on MT8192.

> > 

> > Signed-off-by: Jianjun Wang <jianjun.wang@mediatek.com>

> > Acked-by: Ryder Lee <ryder.lee@mediatek.com>

> > ---

> >  drivers/pci/controller/Kconfig              |   14 +

> >  drivers/pci/controller/Makefile             |    1 +

> >  drivers/pci/controller/pcie-mediatek-gen3.c | 1031 +++++++++++++++++++

> >  3 files changed, 1046 insertions(+)

> >  create mode 100644 drivers/pci/controller/pcie-mediatek-gen3.c

> > 

> > diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig

> > index f18c3725ef80..83daa772595b 100644

> > --- a/drivers/pci/controller/Kconfig

> > +++ b/drivers/pci/controller/Kconfig

> > @@ -239,6 +239,20 @@ config PCIE_MEDIATEK

> >  	  Say Y here if you want to enable PCIe controller support on

> >  	  MediaTek SoCs.

> >  

> > +config PCIE_MEDIATEK_GEN3

> > +	tristate "MediaTek GEN3 PCIe controller"

> > +	depends on ARCH_MEDIATEK || COMPILE_TEST

> > +	depends on OF

> > +	depends on PCI_MSI_IRQ_DOMAIN

> > +	help

> > +	  Adds support for PCIe Gen3 MAC controller for MediaTek SoCs.

> > +	  This PCIe controller provides the capable of Gen3, Gen2 and

> > +	  Gen1 speed, and support up to 256 MSI interrupt numbers for

> > +	  multi-function devices.

> > +

> > +	  Say Y here if you want to enable Gen3 PCIe controller support on

> > +	  MediaTek SoCs.

> > +

> >  config PCIE_TANGO_SMP8759

> >  	bool "Tango SMP8759 PCIe controller (DANGEROUS)"

> >  	depends on ARCH_TANGO && PCI_MSI && OF

> > diff --git a/drivers/pci/controller/Makefile b/drivers/pci/controller/Makefile

> > index bcdbf49ab1e4..9c1b96777597 100644

> > --- a/drivers/pci/controller/Makefile

> > +++ b/drivers/pci/controller/Makefile

> > @@ -27,6 +27,7 @@ obj-$(CONFIG_PCIE_ROCKCHIP) += pcie-rockchip.o

> >  obj-$(CONFIG_PCIE_ROCKCHIP_EP) += pcie-rockchip-ep.o

> >  obj-$(CONFIG_PCIE_ROCKCHIP_HOST) += pcie-rockchip-host.o

> >  obj-$(CONFIG_PCIE_MEDIATEK) += pcie-mediatek.o

> > +obj-$(CONFIG_PCIE_MEDIATEK_GEN3) += pcie-mediatek-gen3.o

> >  obj-$(CONFIG_PCIE_TANGO_SMP8759) += pcie-tango.o

> >  obj-$(CONFIG_VMD) += vmd.o

> >  obj-$(CONFIG_PCIE_BRCMSTB) += pcie-brcmstb.o

> > diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c b/drivers/pci/controller/pcie-mediatek-gen3.c

> > new file mode 100644

> > index 000000000000..dd454cfee373

> > --- /dev/null

> > +++ b/drivers/pci/controller/pcie-mediatek-gen3.c

> > @@ -0,0 +1,1031 @@

> > +// SPDX-License-Identifier: GPL-2.0

> > +/*

> > + * MediaTek PCIe host controller driver.

> > + *

> > + * Copyright (c) 2020 MediaTek Inc.

> > + * Author: Jianjun Wang <jianjun.wang@mediatek.com>

> > + */

> > +

> > +#include <linux/clk.h>

> > +#include <linux/delay.h>

> > +#include <linux/iopoll.h>

> > +#include <linux/irq.h>

> > +#include <linux/irqchip/chained_irq.h>

> > +#include <linux/irqdomain.h>

> > +#include <linux/kernel.h>

> > +#include <linux/module.h>

> > +#include <linux/msi.h>

> > +#include <linux/of_address.h>

> > +#include <linux/of_clk.h>

> > +#include <linux/of_pci.h>

> > +#include <linux/of_platform.h>

> > +#include <linux/pci.h>

> > +#include <linux/phy/phy.h>

> > +#include <linux/platform_device.h>

> > +#include <linux/pm_domain.h>

> > +#include <linux/pm_runtime.h>

> > +#include <linux/reset.h>

> > +

> > +#include "../pci.h"

> > +

> > +#define PCIE_SETTING_REG		0x80

> > +#define PCIE_PCI_IDS_1			0x9c

> > +#define PCI_CLASS(class)		(class << 8)

> > +#define PCIE_RC_MODE			BIT(0)

> > +

> > +#define PCIE_CFGNUM_REG			0x140

> > +#define PCIE_CFG_DEVFN(devfn)		((devfn) & GENMASK(7, 0))

> > +#define PCIE_CFG_BUS(bus)		(((bus) << 8) & GENMASK(15, 8))

> > +#define PCIE_CFG_BYTE_EN(bytes)		(((bytes) << 16) & GENMASK(19, 16))

> > +#define PCIE_CFG_FORCE_BYTE_EN		BIT(20)

> > +#define PCIE_CFG_OFFSET_ADDR		0x1000

> > +#define PCIE_CFG_HEADER(devfn, bus) \

> > +	(PCIE_CFG_DEVFN(devfn) | PCIE_CFG_BUS(bus))

> > +

> > +#define PCIE_CFG_HEADER_FORCE_BE(devfn, bus, bytes) \

> > +	(PCIE_CFG_HEADER(devfn, bus) | PCIE_CFG_BYTE_EN(bytes) \

> > +	 | PCIE_CFG_FORCE_BYTE_EN)

> > +

> > +#define PCIE_RST_CTRL_REG		0x148

> > +#define PCIE_MAC_RSTB			BIT(0)

> > +#define PCIE_PHY_RSTB			BIT(1)

> > +#define PCIE_BRG_RSTB			BIT(2)

> > +#define PCIE_PE_RSTB			BIT(3)

> > +

> > +#define PCIE_MISC_STATUS_REG		0x14C

> > +#define PCIE_LTR_MSG_RECEIVED		BIT(0)

> > +#define PCIE_PCIE_MSG_RECEIVED		BIT(1)

> > +

> > +#define PCIE_LTSSM_STATUS_REG		0x150

> > +#define PCIE_LTSSM_STATE_MASK		GENMASK(28, 24)

> > +#define PCIE_LTSSM_STATE(val)		((val & PCIE_LTSSM_STATE_MASK) >> 24)

> > +#define PCIE_LTSSM_STATE_L0		0x10

> > +#define PCIE_LTSSM_STATE_L1_IDLE	0x13

> > +#define PCIE_LTSSM_STATE_L2_IDLE	0x14

> > +

> > +#define PCIE_LINK_STATUS_REG		0x154

> > +#define PCIE_PORT_LINKUP		BIT(8)

> > +

> > +#define PCIE_MSI_SET_NUM		8

> > +#define PCIE_MSI_IRQS_PER_SET		32

> > +#define PCIE_MSI_IRQS_NUM \

> > +	(PCIE_MSI_IRQS_PER_SET * (PCIE_MSI_SET_NUM))

> > +

> > +#define PCIE_INT_ENABLE_REG		0x180

> > +#define PCIE_MSI_MASK			GENMASK(PCIE_MSI_SET_NUM + 8 - 1, 8)

> > +#define PCIE_MSI_SHIFT			8

> > +#define PCIE_INTX_SHIFT			24

> > +#define PCIE_INTX_MASK			GENMASK(27, 24)

> > +#define PCIE_MSG_MASK			BIT(28)

> > +#define PCIE_AER_MASK			BIT(29)

> > +#define PCIE_PM_MASK			BIT(30)

> > +

> > +#define PCIE_INT_STATUS_REG		0x184

> > +#define PCIE_MSI_SET_ENABLE_REG		0x190

> > +

> > +#define PCIE_LOW_POWER_CTRL_REG		0x194

> > +#define PCIE_DIS_LOWPWR_MASK		GENMASK(3, 0)

> > +#define PCIE_DIS_L0S_MASK		BIT(0)

> > +#define PCIE_DIS_L1_MASK		BIT(1)

> > +#define PCIE_DIS_L11_MASK		BIT(2)

> > +#define PCIE_DIS_L12_MASK		BIT(3)

> > +#define PCIE_FORCE_DIS_LOWPWR		GENMASK(11, 8)

> > +#define PCIE_FORCE_DIS_L0S		BIT(8)

> > +#define PCIE_FORCE_DIS_L1		BIT(9)

> > +#define PCIE_FORCE_DIS_L11		BIT(10)

> > +#define PCIE_FORCE_DIS_L12		BIT(11)

> > +

> > +#define PCIE_ICMD_PM_REG		0x198

> > +#define PCIE_TURN_OFF_LINK		BIT(4)

> > +

> > +#define PCIE_MSI_ADDR_BASE_REG		0xc00

> > +#define PCIE_MSI_SET_OFFSET		0x10

> > +#define PCIE_MSI_STATUS_OFFSET		0x04

> > +#define PCIE_MSI_ENABLE_OFFSET		0x08

> > +

> > +#define PCIE_TRANS_TABLE_BASE_REG	0x800

> > +#define PCIE_ATR_SRC_ADDR_MSB_OFFSET	0x4

> > +#define PCIE_ATR_TRSL_ADDR_LSB_OFFSET	0x8

> > +#define PCIE_ATR_TRSL_ADDR_MSB_OFFSET	0xc

> > +#define PCIE_ATR_TRSL_PARAM_OFFSET	0x10

> > +#define PCIE_ATR_TLB_SET_OFFSET		0x20

> > +

> > +#define PCIE_MAX_TRANS_TABLES		8

> > +#define ATR_EN				BIT(0)

> > +#define ATR_SIZE(size)			((((size) - 1) << 1) & GENMASK(6, 1))

> > +#define ATR_ID(id)			(id & GENMASK(3, 0))

> > +#define ATR_PARAM(param)		(((param) << 16) & GENMASK(27, 16))

> > +

> > +/**

> > + * struct mtk_pcie_msi - MSI information for each set

> > + * @base: IO mapped register base

> > + * @irq: MSI set Interrupt number

> > + * @index: MSI set number

> > + * @msg_addr: MSI message address

> > + * @domain: IRQ domain

> > + */

> > +struct mtk_pcie_msi {

> > +	void __iomem *base;

> > +	unsigned int irq;

> > +	int index;

> > +	phys_addr_t msg_addr;

> > +	struct irq_domain *domain;

> > +};

> > +

> > +/**

> > + * struct mtk_pcie_port - PCIe port information

> > + * @dev: PCIe device

> > + * @base: IO mapped register base

> > + * @reg_base: Physical register base

> > + * @mac_reset: mac reset control

> > + * @phy_reset: phy reset control

> > + * @phy: PHY controller block

> > + * @clks: PCIe clocks

> > + * @num_clks: PCIe clocks count for this port

> > + * @is_suspended: device suspend state

> > + * @irq: PCIe controller interrupt number

> > + * @intx_domain: legacy INTx IRQ domain

> > + * @msi_domain: MSI IRQ domain

> > + * @msi_top_domain: MSI IRQ top domain

> > + * @msi_info: MSI sets information

> > + * @lock: lock protecting IRQ bit map

> > + * @msi_irq_in_use: bit map for assigned MSI IRQ

> > + */

> > +struct mtk_pcie_port {

> > +	struct device *dev;

> > +	void __iomem *base;

> > +	phys_addr_t reg_base;

> > +	struct reset_control *mac_reset;

> > +	struct reset_control *phy_reset;

> > +	struct phy *phy;

> > +	struct clk_bulk_data *clks;

> > +	int num_clks;

> > +	bool is_suspended;

> > +

> > +	int irq;

> > +	struct irq_domain *intx_domain;

> > +	struct irq_domain *msi_domain;

> > +	struct irq_domain *msi_top_domain;

> > +	struct mtk_pcie_msi **msi_info;

> > +	struct mutex lock;

> > +	DECLARE_BITMAP(msi_irq_in_use, PCIE_MSI_IRQS_NUM);

> > +};

> > +

> > +static int mtk_pcie_config_read(struct pci_bus *bus, unsigned int devfn,

> > +				    int where, int size, u32 *val)

> > +{

> > +	struct mtk_pcie_port *port = bus->sysdata;

> > +	int bytes;

> > +

> > +	bytes = ((1 << size) - 1) << (where & 0x3);

> > +	writel(PCIE_CFG_HEADER_FORCE_BE(devfn, bus->number, bytes),

> > +	       port->base + PCIE_CFGNUM_REG);

> 

> Put this in a map_bus() function and then use pci_generic_config_read32 

> and _write32.

> 

> 32-bit only accesses are problematic. Do you require 32-bit accesses for 

> downstream devices or just root port/bus? You can now define child_ops. 


Hi Rob,

Thanks for your review, I will split it to a map_bus() function and
read/write functions with pci_generic_config_read32/write32 in the next
version.

The CPU interface can be 32-bit only accesses, we will enable the
significant bytes which in the TLP header by setting the register of
PCIE_CFGNUM_REG.

Thanks.
> 

> > +

> > +	*val = readl(port->base + PCIE_CFG_OFFSET_ADDR + (where & ~0x3));

> > +

> > +	if (size <= 2)

> > +		*val = (*val >> (8 * (where & 0x3))) & ((1 << (size * 8)) - 1);

> > +

> > +	return PCIBIOS_SUCCESSFUL;

> > +}

> > +

> > +static int mtk_pcie_config_write(struct pci_bus *bus, unsigned int devfn,

> > +				     int where, int size, u32 val)

> > +{

> > +	struct mtk_pcie_port *port = bus->sysdata;

> > +	int bytes;

> > +

> > +	bytes = ((1 << size) - 1) << (where & 0x3);

> > +	writel(PCIE_CFG_HEADER_FORCE_BE(devfn, bus->number, bytes),

> > +	       port->base + PCIE_CFGNUM_REG);

> > +

> > +	if (size <= 2)

> > +		val = (val & ((1 << (size * 8)) - 1)) << ((where & 0x3) * 8);

> > +

> > +	writel(val, port->base + PCIE_CFG_OFFSET_ADDR + (where & ~0x3));

> > +

> > +	return PCIBIOS_SUCCESSFUL;

> > +}

> > +

> > +static struct pci_ops mtk_pcie_ops = {

> > +	.read  = mtk_pcie_config_read,

> > +	.write = mtk_pcie_config_write,

> > +};

> > +

> > +static void mtk_pcie_set_trans_window(void __iomem *reg,

> > +				      resource_size_t cpu_addr,

> > +				      resource_size_t pci_addr, size_t size)

> > +{

> > +	writel(lower_32_bits(cpu_addr) | ATR_SIZE(fls(size) - 1) | ATR_EN, reg);

> > +	writel(upper_32_bits(cpu_addr), reg + PCIE_ATR_SRC_ADDR_MSB_OFFSET);

> > +	writel(lower_32_bits(pci_addr), reg + PCIE_ATR_TRSL_ADDR_LSB_OFFSET);

> > +	writel(upper_32_bits(pci_addr), reg + PCIE_ATR_TRSL_ADDR_MSB_OFFSET);

> > +	writel(ATR_ID(0) | ATR_PARAM(0), reg + PCIE_ATR_TRSL_PARAM_OFFSET);

> > +}

> > +

> > +static int mtk_pcie_set_trans_table(void __iomem *reg,

> > +				    resource_size_t cpu_addr,

> > +				    resource_size_t pci_addr, size_t size,

> > +				    int num)

> 

> Just merge these 2 functions into 1.

> 

> > +{

> > +	void __iomem *table_base;

> > +

> > +	if (num > PCIE_MAX_TRANS_TABLES)

> > +		return -ENODEV;

> > +

> > +	table_base = reg + num * PCIE_ATR_TLB_SET_OFFSET;

> > +	mtk_pcie_set_trans_window(table_base, cpu_addr, pci_addr, size);

> > +

> > +	return 0;

> > +}

> > +

> > +static int mtk_pcie_startup_port(struct mtk_pcie_port *port)

> > +{

> > +	struct resource_entry *entry;

> > +	struct pci_host_bridge *host = pci_host_bridge_from_priv(port);

> > +	unsigned int table_index = 0;

> > +	int err;

> > +	u32 val;

> > +

> > +	/* Set as RC mode */

> > +	val = readl(port->base + PCIE_SETTING_REG);

> > +	val |= PCIE_RC_MODE;

> > +	writel(val, port->base + PCIE_SETTING_REG);

> > +

> > +	/* Set class code */

> > +	val = readl(port->base + PCIE_PCI_IDS_1);

> > +	val &= ~GENMASK(31, 8);

> > +	val |= PCI_CLASS(PCI_CLASS_BRIDGE_PCI << 8);

> > +	writel(val, port->base + PCIE_PCI_IDS_1);

> > +

> > +	/* Assert all reset signals */

> > +	val = readl(port->base + PCIE_RST_CTRL_REG);

> > +	val |= PCIE_MAC_RSTB | PCIE_PHY_RSTB | PCIE_BRG_RSTB | PCIE_PE_RSTB;

> > +	writel(val, port->base + PCIE_RST_CTRL_REG);

> > +

> > +	/* De-assert reset signals*/

> > +	val &= ~(PCIE_MAC_RSTB | PCIE_PHY_RSTB | PCIE_BRG_RSTB);

> > +	writel(val, port->base + PCIE_RST_CTRL_REG);

> > +

> > +	/* Delay 100ms to wait the reference clocks become stable */

> > +	usleep_range(100 * 1000, 120 * 1000);

> > +

> > +	/* De-assert pe reset*/

> > +	val &= ~PCIE_PE_RSTB;

> > +	writel(val, port->base + PCIE_RST_CTRL_REG);

> > +

> > +	/* Check if the link is up or not */

> > +	err = readl_poll_timeout(port->base + PCIE_LINK_STATUS_REG, val,

> > +			!!(val & PCIE_PORT_LINKUP), 20,

> > +			50 * USEC_PER_MSEC);

> > +	if (err) {

> > +		val = readl(port->base + PCIE_LTSSM_STATUS_REG);

> > +		dev_dbg(port->dev, "ltssm reg val: %#x\n", val);

> > +		return err;

> > +	}

> > +

> > +	/* Set PCIe translation windows */

> > +	resource_list_for_each_entry(entry, &host->windows) {

> > +		unsigned long type = resource_type(entry->res);

> > +		struct resource *res = NULL;

> > +		resource_size_t cpu_addr;

> > +		resource_size_t pci_addr;

> > +

> > +		if (!(type & (IORESOURCE_MEM | IORESOURCE_IO)))

> > +			continue;

> > +

> > +		res = entry->res;

> > +		cpu_addr = res->start;

> > +		pci_addr = res->start - entry->offset;

> > +		mtk_pcie_set_trans_table(port->base + PCIE_TRANS_TABLE_BASE_REG,

> > +					 cpu_addr, pci_addr, resource_size(res),

> > +					 table_index);

> 

> You don't distinguish IO regions in any way. How can that work?


The IO regions need additional settings, I will fix it in the next
version.
> 

> > +

> > +		dev_dbg(port->dev, "Set %s trans window[%d]: cpu_addr = %#llx, pci_addr = %#llx, size = %#llx\n",

> > +			(!!(type & IORESOURCE_MEM) ? "MEM" : "IO"), table_index,

> > +			cpu_addr, pci_addr, resource_size(res));

> > +

> > +		table_index++;

> > +	}

> > +

> > +	return 0;

> > +}

> > +

> > +static inline struct mtk_pcie_msi *mtk_get_msi_info(struct mtk_pcie_port *port,

> > +						    unsigned long hwirq)

> > +{

> > +	return port->msi_info[hwirq / PCIE_MSI_IRQS_PER_SET];

> > +}

> > +

> > +static int mtk_pcie_set_affinity(struct irq_data *data,

> > +				 const struct cpumask *mask, bool force)

> > +{

> > +	struct mtk_pcie_port *port = irq_data_get_irq_chip_data(data);

> > +	int ret;

> > +

> > +	ret = irq_set_affinity_hint(port->irq, mask);

> > +	if (ret)

> > +		return ret;

> > +

> > +	irq_data_update_effective_affinity(data, mask);

> > +

> > +	return 0;

> > +}

> > +

> > +static void mtk_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)

> > +{

> > +	struct mtk_pcie_msi *msi_info;

> > +	struct mtk_pcie_port *port = irq_data_get_irq_chip_data(data);

> > +

> > +	msi_info = mtk_get_msi_info(port, data->hwirq);

> > +

> > +	msg->address_hi = 0;

> > +	msg->address_lo = lower_32_bits(msi_info->msg_addr);

> > +

> > +	msg->data = data->hwirq;

> > +	dev_dbg(port->dev, "msi#%#lx address_hi %#x address_lo %#x data %d\n",

> > +		data->hwirq, msg->address_hi, msg->address_lo, msg->data);

> > +}

> > +

> > +static void mtk_msi_irq_ack(struct irq_data *data)

> > +{

> > +	struct mtk_pcie_msi *msi_info;

> > +	struct mtk_pcie_port *port = irq_data_get_irq_chip_data(data);

> > +	unsigned long hwirq;

> > +

> > +	msi_info = mtk_get_msi_info(port, data->hwirq);

> > +

> > +	hwirq =	data->hwirq % PCIE_MSI_IRQS_PER_SET;

> > +

> > +	writel(1 << hwirq, msi_info->base + PCIE_MSI_STATUS_OFFSET);

> > +}

> > +

> > +static void mtk_msi_irq_mask(struct irq_data *data)

> > +{

> > +	struct mtk_pcie_msi *msi_info;

> > +	struct mtk_pcie_port *port = irq_data_get_irq_chip_data(data);

> > +	u32 val;

> > +

> > +	msi_info = mtk_get_msi_info(port, data->hwirq);

> > +	val = readl(msi_info->base + PCIE_MSI_ENABLE_OFFSET);

> > +	val &= ~(1 << data->hwirq);

> 

> Use BIT() macro here and elsewhere.

> 

> > +	writel(val, msi_info->base + PCIE_MSI_ENABLE_OFFSET);

> > +

> > +	pci_msi_mask_irq(data);

> > +}

> > +

> > +static void mtk_msi_irq_unmask(struct irq_data *data)

> > +{

> > +	struct mtk_pcie_msi *msi_info;

> > +	struct mtk_pcie_port *port = irq_data_get_irq_chip_data(data);

> > +	u32 val;

> > +

> > +	msi_info = mtk_get_msi_info(port, data->hwirq);

> > +	val = readl(msi_info->base + PCIE_MSI_ENABLE_OFFSET);

> > +	val |= (1 << data->hwirq);

> > +	writel(val, msi_info->base + PCIE_MSI_ENABLE_OFFSET);

> > +

> > +	pci_msi_unmask_irq(data);

> > +}

> > +

> > +static struct irq_chip mtk_msi_irq_chip = {

> > +	.irq_ack		= mtk_msi_irq_ack,

> > +	.irq_compose_msi_msg	= mtk_compose_msi_msg,

> > +	.irq_mask		= mtk_msi_irq_mask,

> > +	.irq_unmask		= mtk_msi_irq_unmask,

> > +	.irq_set_affinity	= mtk_pcie_set_affinity,

> > +	.name			= "PCIe",

> > +};

> > +

> > +static irq_hw_number_t mtk_pcie_msi_get_hwirq(struct msi_domain_info *info,

> > +					      msi_alloc_info_t *arg)

> > +{

> > +	struct msi_desc *entry = arg->desc;

> > +	struct mtk_pcie_port *port = info->chip_data;

> > +	int hwirq;

> > +

> > +	mutex_lock(&port->lock);

> > +

> > +	hwirq = bitmap_find_free_region(port->msi_irq_in_use, PCIE_MSI_IRQS_NUM,

> > +			order_base_2(entry->nvec_used));

> > +	if (hwirq < 0) {

> > +		mutex_unlock(&port->lock);

> > +		return -ENOSPC;

> > +	}

> > +

> > +	mutex_unlock(&port->lock);

> > +

> > +	return hwirq;

> > +}

> > +

> > +static void mtk_pcie_msi_free(struct irq_domain *domain,

> > +			      struct msi_domain_info *info, unsigned int virq)

> > +{

> > +	struct irq_data *data = irq_domain_get_irq_data(domain, virq);

> > +	struct mtk_pcie_port *port = irq_data_get_irq_chip_data(data);

> > +

> > +	mutex_lock(&port->lock);

> > +

> > +	bitmap_clear(port->msi_irq_in_use, data->hwirq, 1);

> > +

> > +	mutex_unlock(&port->lock);

> > +}

> > +

> > +static struct msi_domain_ops mtk_msi_domain_ops = {

> > +	.get_hwirq	= mtk_pcie_msi_get_hwirq,

> > +	.msi_free	= mtk_pcie_msi_free,

> > +};

> > +

> > +static struct msi_domain_info mtk_msi_domain_info = {

> > +	.flags		= (MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_PCI_MSIX |

> > +			   MSI_FLAG_USE_DEF_CHIP_OPS | MSI_FLAG_MULTI_PCI_MSI),

> > +	.chip		= &mtk_msi_irq_chip,

> > +	.ops		= &mtk_msi_domain_ops,

> > +	.handler	= handle_edge_irq,

> > +	.handler_name	= "MSI",

> > +};

> > +

> > +static void mtk_msi_top_irq_eoi(struct irq_data *data)

> > +{

> > +	struct mtk_pcie_port *port = irq_data_get_irq_chip_data(data);

> > +	unsigned long msi_irq = data->hwirq + PCIE_MSI_SHIFT;

> > +

> > +	writel(1 << msi_irq, port->base + PCIE_INT_STATUS_REG);

> > +}

> > +

> > +static struct irq_chip mtk_msi_top_irq_chip = {

> > +	.irq_eoi	= mtk_msi_top_irq_eoi,

> > +	.name		= "PCIe",

> > +};

> > +

> > +static void mtk_pcie_msi_handler(struct irq_desc *desc)

> > +{

> > +	struct mtk_pcie_msi *msi_info = irq_desc_get_handler_data(desc);

> > +	struct irq_chip *irqchip = irq_desc_get_chip(desc);

> > +	unsigned long msi_enable, msi_status;

> > +	unsigned int virq;

> > +	irq_hw_number_t bit, hwirq;

> > +

> > +	chained_irq_enter(irqchip, desc);

> > +

> > +	msi_enable = readl(msi_info->base + PCIE_MSI_ENABLE_OFFSET);

> > +	while ((msi_status = readl(msi_info->base + PCIE_MSI_STATUS_OFFSET))) {

> > +		msi_status &= msi_enable;

> > +		for_each_set_bit(bit, &msi_status, PCIE_MSI_IRQS_PER_SET) {

> > +			hwirq = bit + msi_info->index * PCIE_MSI_IRQS_PER_SET;

> > +			virq = irq_find_mapping(msi_info->domain, hwirq);

> > +			generic_handle_irq(virq);

> > +		}

> > +	}

> > +

> > +	chained_irq_exit(irqchip, desc);

> > +}

> > +

> > +static int mtk_msi_top_domain_map(struct irq_domain *domain,

> > +				    unsigned int virq, irq_hw_number_t hwirq)

> > +{

> > +	struct mtk_pcie_port *port = domain->host_data;

> > +	struct mtk_pcie_msi *msi_info = port->msi_info[hwirq];

> > +

> > +	irq_domain_set_info(domain, virq, hwirq,

> > +			    &mtk_msi_top_irq_chip, domain->host_data,

> > +			    mtk_pcie_msi_handler, msi_info, NULL);

> > +

> > +	return 0;

> > +}

> > +

> > +static const struct irq_domain_ops mtk_msi_top_domain_ops = {

> > +	.map = mtk_msi_top_domain_map,

> > +};

> > +

> > +static void mtk_intx_mask(struct irq_data *data)

> > +{

> > +	struct mtk_pcie_port *port = irq_data_get_irq_chip_data(data);

> > +	u32 val;

> > +

> > +	val = readl(port->base + PCIE_INT_ENABLE_REG);

> > +	val &= ~(1 << (data->hwirq + PCIE_INTX_SHIFT));

> > +	writel(val, port->base + PCIE_INT_ENABLE_REG);

> > +}

> > +

> > +static void mtk_intx_unmask(struct irq_data *data)

> > +{

> > +	struct mtk_pcie_port *port = irq_data_get_irq_chip_data(data);

> > +	u32 val;

> > +

> > +	val = readl(port->base + PCIE_INT_ENABLE_REG);

> > +	val |= 1 << (data->hwirq + PCIE_INTX_SHIFT);

> > +	writel(val, port->base + PCIE_INT_ENABLE_REG);

> > +}

> > +

> > +static void mtk_intx_eoi(struct irq_data *data)

> > +{

> > +	struct mtk_pcie_port *port = irq_data_get_irq_chip_data(data);

> > +	unsigned long hwirq;

> > +

> > +	/**

> > +	 * As an emulated level irq, its interrupt status will be remained

> > +	 * until receive the corresponding message of de-assert, hence that

> > +	 * the status can only be cleared when the interrupt has been serviced.

> > +	 */

> > +	hwirq = data->hwirq + PCIE_INTX_SHIFT;

> > +	writel(1 << hwirq, port->base + PCIE_INT_STATUS_REG);

> > +}

> > +

> > +static struct irq_chip mtk_intx_irq_chip = {

> > +	.irq_mask		= mtk_intx_mask,

> > +	.irq_unmask		= mtk_intx_unmask,

> > +	.irq_eoi		= mtk_intx_eoi,

> > +	.irq_set_affinity	= mtk_pcie_set_affinity,

> > +	.name			= "PCIe",

> > +};

> > +

> > +static int mtk_pcie_intx_map(struct irq_domain *domain, unsigned int irq,

> > +			     irq_hw_number_t hwirq)

> > +{

> > +	irq_set_chip_and_handler_name(irq, &mtk_intx_irq_chip,

> > +				      handle_fasteoi_irq, "INTx");

> > +	irq_set_chip_data(irq, domain->host_data);

> > +

> > +	return 0;

> > +}

> > +

> > +static const struct irq_domain_ops intx_domain_ops = {

> > +	.map = mtk_pcie_intx_map,

> > +};

> > +

> > +static int mtk_pcie_init_irq_domains(struct mtk_pcie_port *port,

> > +				     struct device_node *node)

> > +{

> > +	struct device *dev = port->dev;

> > +	struct device_node *intc_node;

> > +	struct fwnode_handle *fwnode = of_node_to_fwnode(node);

> > +	struct mtk_pcie_msi *msi_info;

> > +	struct msi_domain_info *info;

> > +	int i, ret;

> > +

> > +	/* Setup INTx */

> > +	intc_node = of_get_child_by_name(node, "interrupt-controller");

> > +	if (!intc_node) {

> > +		dev_notice(dev, "Missing PCIe Intc node\n");

> > +		return -ENODEV;

> > +	}

> > +

> > +	port->intx_domain = irq_domain_add_linear(intc_node, PCI_NUM_INTX,

> > +						  &intx_domain_ops, port);

> > +	if (!port->intx_domain) {

> > +		dev_notice(dev, "failed to get INTx IRQ domain\n");

> > +		return -ENODEV;

> > +	}

> > +

> > +	/* Setup MSI */

> > +	mutex_init(&port->lock);

> > +

> > +	info = devm_kzalloc(dev, sizeof(*info), GFP_KERNEL);

> > +	if (!info)

> > +		return -ENOMEM;

> > +

> > +	memcpy(info, &mtk_msi_domain_info, sizeof(*info));

> > +	info->chip_data = port;

> > +

> > +	port->msi_domain = pci_msi_create_irq_domain(fwnode, info, NULL);

> > +	if (!port->msi_domain) {

> > +		dev_info(dev, "failed to create msi domain\n");

> > +		ret = -ENODEV;

> > +		goto err_msi_domain;

> > +	}

> > +

> > +	/* Enable MSI and setup pcie domains */

> > +	port->msi_top_domain = irq_domain_add_hierarchy(NULL, 0, 0, node,

> > +							&mtk_msi_top_domain_ops,

> > +							port);

> > +	if (!port->msi_top_domain) {

> > +		dev_info(dev, "failed to create msi top domain\n");

> > +		ret = -ENODEV;

> > +		goto err_msi_top_domain;

> > +	}

> > +

> > +	port->msi_info = devm_kzalloc(dev, PCIE_MSI_SET_NUM, GFP_KERNEL);

> > +	if (!port->msi_info) {

> > +		ret = -ENOMEM;

> > +		goto err_msi_info;

> > +	}

> > +

> > +	for (i = 0; i < PCIE_MSI_SET_NUM; i++) {

> > +		int offset = i * PCIE_MSI_SET_OFFSET;

> > +		u32 val;

> > +

> > +		msi_info = devm_kzalloc(dev, sizeof(*msi_info), GFP_KERNEL);

> > +		if (!msi_info) {

> > +			ret = -ENOMEM;

> > +			goto err_msi_set;

> > +		}

> > +

> > +		msi_info->base = port->base + PCIE_MSI_ADDR_BASE_REG + offset;

> > +		msi_info->msg_addr = port->reg_base + PCIE_MSI_ADDR_BASE_REG +

> > +				     offset;

> > +

> > +		writel(lower_32_bits(msi_info->msg_addr), msi_info->base);

> > +

> > +		msi_info->index = i;

> > +		msi_info->domain = port->msi_domain;

> > +

> > +		port->msi_info[i] = msi_info;

> > +

> > +		/* Alloc irq for each msi set */

> > +		msi_info->irq = irq_create_mapping(port->msi_top_domain, i);

> > +		if (!msi_info->irq) {

> > +			dev_info(dev, "allocate msi top irq failed\n");

> > +			ret = -ENOSPC;

> > +			goto err_msi_set;

> > +		}

> > +

> > +		val = readl(port->base + PCIE_INT_ENABLE_REG);

> > +		val |= (1 << (i + PCIE_MSI_SHIFT));

> > +		writel(val, port->base + PCIE_INT_ENABLE_REG);

> > +

> > +		val = readl(port->base + PCIE_MSI_SET_ENABLE_REG);

> > +		val |= (1 << i);

> > +		writel(val, port->base + PCIE_MSI_SET_ENABLE_REG);

> > +	}

> > +

> > +	return 0;

> > +

> > +err_msi_set:

> > +	while (--i >= 0) {

> > +		msi_info = port->msi_info[i];

> > +		irq_dispose_mapping(msi_info->irq);

> > +	}

> > +err_msi_info:

> > +	irq_domain_remove(port->msi_top_domain);

> > +err_msi_top_domain:

> > +	irq_domain_remove(port->msi_domain);

> > +err_msi_domain:

> > +	irq_domain_remove(port->intx_domain);

> > +

> > +	return ret;

> > +}

> > +

> > +static void mtk_pcie_irq_teardown(struct mtk_pcie_port *port)

> > +{

> > +	struct mtk_pcie_msi *msi_info;

> > +	int i;

> > +

> > +	irq_set_chained_handler_and_data(port->irq, NULL, NULL);

> > +

> > +	if (port->intx_domain)

> > +		irq_domain_remove(port->intx_domain);

> > +

> > +	if (port->msi_domain)

> > +		irq_domain_remove(port->msi_domain);

> > +

> > +	if (port->msi_top_domain) {

> > +		for (i = 0; i < PCIE_MSI_SET_NUM; i++) {

> > +			msi_info = port->msi_info[i];

> > +			irq_dispose_mapping(msi_info->irq);

> > +		}

> > +

> > +		irq_domain_remove(port->msi_top_domain);

> > +	}

> > +

> > +	irq_dispose_mapping(port->irq);

> > +}

> > +

> > +static void mtk_pcie_irq_handler(struct irq_desc *desc)

> > +{

> > +	struct mtk_pcie_port *port = irq_desc_get_handler_data(desc);

> > +	struct irq_chip *irqchip = irq_desc_get_chip(desc);

> > +	unsigned long status;

> > +	unsigned int virq;

> > +	irq_hw_number_t irq_bit = PCIE_INTX_SHIFT;

> > +

> > +	chained_irq_enter(irqchip, desc);

> > +

> > +	status = readl(port->base + PCIE_INT_STATUS_REG);

> > +	if (status & PCIE_INTX_MASK) {

> > +		for_each_set_bit_from(irq_bit, &status, PCI_NUM_INTX +

> > +				      PCIE_INTX_SHIFT) {

> > +			virq = irq_find_mapping(port->intx_domain,

> > +						irq_bit - PCIE_INTX_SHIFT);

> > +			generic_handle_irq(virq);

> > +		}

> > +	}

> > +

> > +	if (status & PCIE_MSI_MASK) {

> > +		irq_bit = PCIE_MSI_SHIFT;

> > +		for_each_set_bit_from(irq_bit, &status, PCIE_MSI_SET_NUM +

> > +				      PCIE_MSI_SHIFT) {

> > +			virq = irq_find_mapping(port->msi_top_domain,

> > +						irq_bit - PCIE_MSI_SHIFT);

> > +			generic_handle_irq(virq);

> > +		}

> > +	}

> > +

> > +	chained_irq_exit(irqchip, desc);

> > +}

> > +

> > +static int mtk_pcie_setup_irq(struct mtk_pcie_port *port,

> > +			      struct device_node *node)

> > +{

> > +	struct device *dev = port->dev;

> > +	struct platform_device *pdev = to_platform_device(dev);

> > +	int err;

> > +

> > +	err = mtk_pcie_init_irq_domains(port, node);

> > +	if (err) {

> > +		dev_notice(dev, "failed to init PCIe IRQ domain\n");

> > +		return err;

> > +	}

> > +

> > +	port->irq = platform_get_irq(pdev, 0);

> > +	if (port->irq < 0)

> > +		return port->irq;

> > +

> > +	irq_set_chained_handler_and_data(port->irq, mtk_pcie_irq_handler, port);

> > +

> > +	return 0;

> > +}

> > +

> > +static int mtk_pcie_clk_init(struct mtk_pcie_port *port)

> > +{

> > +	int ret;

> > +

> > +	port->num_clks = devm_clk_bulk_get_all(port->dev, &port->clks);

> > +	if (port->num_clks < 0) {

> > +		dev_notice(port->dev, "failed to get pcie clock\n");

> > +		return port->num_clks;

> > +	}

> > +

> > +	ret = clk_bulk_prepare_enable(port->num_clks, port->clks);

> > +	if (ret) {

> > +		dev_notice(port->dev, "failed to enable pcie clocks\n");

> > +		return ret;

> > +	}

> > +

> > +	return 0;

> > +}

> > +

> > +static int mtk_pcie_power_up(struct mtk_pcie_port *port)

> > +{

> > +	struct device *dev = port->dev;

> > +	int err;

> > +

> > +	port->phy_reset = devm_reset_control_get_optional_exclusive(dev, "phy");

> > +	if (IS_ERR(port->phy_reset))

> > +		return PTR_ERR(port->phy_reset);

> > +

> > +	/* PHY power on and enable pipe clock */

> > +	port->phy = devm_phy_optional_get(dev, "pcie-phy");

> > +	if (IS_ERR(port->phy))

> > +		return PTR_ERR(port->phy);

> > +

> > +	reset_control_deassert(port->phy_reset);

> > +

> > +	err = phy_power_on(port->phy);

> > +	if (err) {

> > +		dev_notice(dev, "failed to power on pcie phy\n");

> > +		goto err_phy_on;

> > +	}

> > +

> > +	err = phy_init(port->phy);

> > +	if (err) {

> > +		dev_notice(dev, "failed to initialize pcie phy\n");

> > +		goto err_phy_init;

> > +	}

> > +

> > +	port->mac_reset = devm_reset_control_get_optional_exclusive(dev, "mac");

> > +	if (IS_ERR(port->mac_reset)) {

> > +		err = PTR_ERR(port->mac_reset);

> > +		goto err_mac_rst;

> > +	}

> > +

> > +	reset_control_deassert(port->mac_reset);

> > +

> > +	/* MAC power on and enable transaction layer clocks */

> > +	pm_runtime_enable(dev);

> > +	pm_runtime_get_sync(dev);

> > +

> > +	err = mtk_pcie_clk_init(port);

> > +	if (err) {

> > +		dev_notice(dev, "clock init failed\n");

> > +		goto err_clk_init;

> > +	}

> > +

> > +	return 0;

> > +

> > +err_clk_init:

> > +	pm_runtime_put_sync(dev);

> > +	pm_runtime_disable(dev);

> > +	reset_control_assert(port->mac_reset);

> > +err_mac_rst:

> > +	phy_exit(port->phy);

> > +err_phy_init:

> > +	phy_power_off(port->phy);

> > +err_phy_on:

> > +	reset_control_assert(port->phy_reset);

> > +

> > +	return err;

> > +}

> > +

> > +static void mtk_pcie_power_down(struct mtk_pcie_port *port)

> > +{

> > +	clk_bulk_disable_unprepare(port->num_clks, port->clks);

> > +

> > +	pm_runtime_put_sync(port->dev);

> > +	pm_runtime_disable(port->dev);

> > +	reset_control_assert(port->mac_reset);

> > +

> > +	phy_power_off(port->phy);

> > +	phy_exit(port->phy);

> > +	reset_control_assert(port->phy_reset);

> > +}

> > +

> > +static int mtk_pcie_setup(struct mtk_pcie_port *port)

> > +{

> > +	struct device *dev = port->dev;

> > +	struct platform_device *pdev = to_platform_device(dev);

> > +	struct resource *regs;

> > +	int err;

> > +

> > +	regs = platform_get_resource_byname(pdev, IORESOURCE_MEM, "pcie-mac");

> > +	port->base = devm_ioremap_resource(dev, regs);

> > +	if (IS_ERR(port->base)) {

> > +		dev_notice(dev, "failed to map register base\n");

> > +		return PTR_ERR(port->base);

> > +	}

> > +

> > +	port->reg_base = regs->start;

> > +

> > +	/* Don't touch the hardware registers before power up */

> > +	err = mtk_pcie_power_up(port);

> > +	if (err)

> > +		return err;

> > +

> > +	/* Try link up */

> > +	err = mtk_pcie_startup_port(port);

> > +	if (err) {

> > +		dev_notice(dev, "PCIe link down\n");

> > +		goto err_setup;

> > +	}

> > +

> > +	err = mtk_pcie_setup_irq(port, dev->of_node);

> > +	if (err)

> > +		goto err_setup;

> > +

> > +	dev_info(dev, "PCIe link up success!\n");

> > +

> > +	return 0;

> > +

> > +err_setup:

> > +	mtk_pcie_power_down(port);

> > +

> > +	return err;

> > +}

> > +

> > +static int mtk_pcie_probe(struct platform_device *pdev)

> > +{

> > +	struct device *dev = &pdev->dev;

> > +	struct mtk_pcie_port *port;

> > +	struct pci_host_bridge *host;

> > +	int err;

> > +

> > +	host = devm_pci_alloc_host_bridge(dev, sizeof(*port));

> > +	if (!host)

> > +		return -ENOMEM;

> > +

> > +	port = pci_host_bridge_priv(host);

> > +

> > +	port->dev = dev;

> > +	platform_set_drvdata(pdev, port);

> > +

> > +	err = mtk_pcie_setup(port);

> > +	if (err)

> > +		return err;

> > +

> > +	host->ops = &mtk_pcie_ops;

> > +	host->sysdata = port;

> > +

> > +	err = pci_host_probe(host);

> > +	if (err) {

> > +		mtk_pcie_power_down(port);

> 

> mtk_pcie_irq_teardown needed here?


Yes, I will add this function in the next version.
> 

> > +		return err;

> > +	}

> > +

> > +	return 0;

> > +}

> > +

> > +static int mtk_pcie_remove(struct platform_device *pdev)

> > +{

> > +	struct mtk_pcie_port *port = platform_get_drvdata(pdev);

> > +	struct pci_host_bridge *host = pci_host_bridge_from_priv(port);

> > +

> > +	pci_lock_rescan_remove();

> > +	pci_stop_root_bus(host->bus);

> > +	pci_remove_root_bus(host->bus);

> > +	pci_unlock_rescan_remove();

> > +

> > +	mtk_pcie_irq_teardown(port);

> > +	mtk_pcie_power_down(port);

> > +

> > +	return 0;

> > +}

> > +

> > +static int __maybe_unused mtk_pcie_turn_off_link(struct mtk_pcie_port *port)

> > +{

> > +	u32 val;

> > +

> > +	val = readl(port->base + PCIE_ICMD_PM_REG);

> > +	val |= PCIE_TURN_OFF_LINK;

> > +	writel(val, port->base + PCIE_ICMD_PM_REG);

> > +

> > +	/* Check the link is L2 */

> > +	return readl_poll_timeout(port->base + PCIE_LTSSM_STATUS_REG, val,

> > +				  (PCIE_LTSSM_STATE(val) ==

> > +				   PCIE_LTSSM_STATE_L2_IDLE), 20,

> > +				   50 * USEC_PER_MSEC);

> > +}

> > +

> > +static int __maybe_unused mtk_pcie_suspend_noirq(struct device *dev)

> > +{

> > +	struct mtk_pcie_port *port = dev_get_drvdata(dev);

> > +	int err;

> > +	u32 val;

> > +

> > +	if (port->is_suspended)

> > +		return 0;

> 

> I don't think this variable is needed. The core should track calling 

> suspend/resume.


OK, I will remove it in the next version.
> 

> > +

> > +	/* Trigger link to L2 state */

> > +	err = mtk_pcie_turn_off_link(port);

> > +	if (err) {

> > +		dev_notice(port->dev, "can not enter L2 state\n");

> 

> Seems more like an error.

> 

> > +		goto power_off;

> > +	}

> > +

> > +	/* Pull down the PERST# pin */

> > +	val = readl(port->base + PCIE_RST_CTRL_REG);

> > +	val |= PCIE_PE_RSTB;

> > +	writel(val, port->base + PCIE_RST_CTRL_REG);

> > +

> > +	dev_info(port->dev, "enter L2 state success");

> 

> Seems like debug.

> 

> > +

> > +power_off:

> > +	clk_bulk_disable_unprepare(port->num_clks, port->clks);

> > +

> > +	phy_power_off(port->phy);

> > +

> > +	port->is_suspended = true;

> > +

> > +	return 0;

> > +}

> > +

> > +static int __maybe_unused mtk_pcie_resume_noirq(struct device *dev)

> > +{

> > +	struct mtk_pcie_port *port = dev_get_drvdata(dev);

> > +	int err;

> > +

> > +	if (!port->is_suspended)

> > +		return 0;

> > +

> > +	phy_power_on(port->phy);

> > +

> > +	err = clk_bulk_prepare_enable(port->num_clks, port->clks);

> > +	if (err) {

> > +		dev_dbg(dev, "failed to enable pcie clocks\n");

> > +		return err;

> > +	}

> > +

> > +	err = mtk_pcie_startup_port(port);

> > +	if (err) {

> > +		dev_notice(port->dev, "resume failed\n");

> > +		return err;

> > +	}

> > +

> > +	port->is_suspended = false;

> > +

> > +	dev_info(port->dev, "resume done\n");

> 

> Debug...

> 

> > +

> > +	return 0;

> > +}

> > +

> > +static const struct dev_pm_ops mtk_pcie_pm_ops = {

> > +	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(mtk_pcie_suspend_noirq,

> > +				      mtk_pcie_resume_noirq)

> > +};

> > +

> > +static const struct of_device_id mtk_pcie_of_match[] = {

> > +	{ .compatible = "mediatek,mt8192-pcie" },

> > +	{},

> > +};

> > +

> > +static struct platform_driver mtk_pcie_driver = {

> > +	.probe = mtk_pcie_probe,

> > +	.remove = mtk_pcie_remove,

> > +	.driver = {

> > +		.name = "mtk-pcie",

> > +		.of_match_table = mtk_pcie_of_match,

> > +		.pm = &mtk_pcie_pm_ops,

> > +	},

> > +};

> > +

> > +module_platform_driver(mtk_pcie_driver);

> > +MODULE_LICENSE("GPL v2");

> > -- 

> > 2.25.1

> > _______________________________________________

> > linux-arm-kernel mailing list

> > linux-arm-kernel@lists.infradead.org

> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On Thu, 2020-11-19 at 14:28 -0600, Bjorn Helgaas wrote:
> "Add new generation" really contains no information.  And "mediatek"

> is already used for the pcie-mediatek.c driver, so we should have a

> new tag for this new driver.  Include useful information in the

> subject, e.g.,

> 

>   PCI: mediatek-gen3: Add MediaTek Gen3 driver for MT8192

> 

> On Wed, Nov 18, 2020 at 04:29:34PM +0800, Jianjun Wang wrote:

> > MediaTek's PCIe host controller has three generation HWs, the new

> > generation HW is an individual bridge, it supoorts Gen3 speed and

> > up to 256 MSI interrupt numbers for multi-function devices.

> 

> s/supoorts/supports/

> 

> > Add support for new Gen3 controller which can be found on MT8192.

> > 

> > Signed-off-by: Jianjun Wang <jianjun.wang@mediatek.com>

> > Acked-by: Ryder Lee <ryder.lee@mediatek.com>

> > ---

> >  drivers/pci/controller/Kconfig              |   14 +

> >  drivers/pci/controller/Makefile             |    1 +

> >  drivers/pci/controller/pcie-mediatek-gen3.c | 1031 +++++++++++++++++++

> >  3 files changed, 1046 insertions(+)

> >  create mode 100644 drivers/pci/controller/pcie-mediatek-gen3.c

> > 

> > diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig

> > index f18c3725ef80..83daa772595b 100644

> > --- a/drivers/pci/controller/Kconfig

> > +++ b/drivers/pci/controller/Kconfig

> > @@ -239,6 +239,20 @@ config PCIE_MEDIATEK

> >  	  Say Y here if you want to enable PCIe controller support on

> >  	  MediaTek SoCs.

> >  

> > +config PCIE_MEDIATEK_GEN3

> > +	tristate "MediaTek GEN3 PCIe controller"

> 

> s/GEN3/Gen3/

> 

> > +	depends on ARCH_MEDIATEK || COMPILE_TEST

> > +	depends on OF

> 

> Do you really need "depends on OF"?  There are stubs to cover most

> interfaces even when CONFIG_OF is not set.  Depending on OF reduces

> compile-test coverage.

> 

> > +	depends on PCI_MSI_IRQ_DOMAIN

> > +	help

> > +	  Adds support for PCIe Gen3 MAC controller for MediaTek SoCs.

> > +	  This PCIe controller provides the capable of Gen3, Gen2 and

> 

> "provides the capable of" doesn't parse.

> 

> > +	  Gen1 speed, and support up to 256 MSI interrupt numbers for

> > +	  multi-function devices.

> > +

> > +	  Say Y here if you want to enable Gen3 PCIe controller support on

> > +	  MediaTek SoCs.

> > +

> >  config PCIE_TANGO_SMP8759

> >  	bool "Tango SMP8759 PCIe controller (DANGEROUS)"

> >  	depends on ARCH_TANGO && PCI_MSI && OF

> > diff --git a/drivers/pci/controller/Makefile b/drivers/pci/controller/Makefile

> > index bcdbf49ab1e4..9c1b96777597 100644

> > --- a/drivers/pci/controller/Makefile

> > +++ b/drivers/pci/controller/Makefile

> > @@ -27,6 +27,7 @@ obj-$(CONFIG_PCIE_ROCKCHIP) += pcie-rockchip.o

> >  obj-$(CONFIG_PCIE_ROCKCHIP_EP) += pcie-rockchip-ep.o

> >  obj-$(CONFIG_PCIE_ROCKCHIP_HOST) += pcie-rockchip-host.o

> >  obj-$(CONFIG_PCIE_MEDIATEK) += pcie-mediatek.o

> > +obj-$(CONFIG_PCIE_MEDIATEK_GEN3) += pcie-mediatek-gen3.o

> >  obj-$(CONFIG_PCIE_TANGO_SMP8759) += pcie-tango.o

> >  obj-$(CONFIG_VMD) += vmd.o

> >  obj-$(CONFIG_PCIE_BRCMSTB) += pcie-brcmstb.o

> > diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c b/drivers/pci/controller/pcie-mediatek-gen3.c

> > new file mode 100644

> > index 000000000000..dd454cfee373

> > --- /dev/null

> > +++ b/drivers/pci/controller/pcie-mediatek-gen3.c

> > @@ -0,0 +1,1031 @@

> > +// SPDX-License-Identifier: GPL-2.0

> > +/*

> > + * MediaTek PCIe host controller driver.

> > + *

> > + * Copyright (c) 2020 MediaTek Inc.

> > + * Author: Jianjun Wang <jianjun.wang@mediatek.com>

> > + */

> > +

> > +#include <linux/clk.h>

> > +#include <linux/delay.h>

> > +#include <linux/iopoll.h>

> > +#include <linux/irq.h>

> > +#include <linux/irqchip/chained_irq.h>

> > +#include <linux/irqdomain.h>

> > +#include <linux/kernel.h>

> > +#include <linux/module.h>

> > +#include <linux/msi.h>

> > +#include <linux/of_address.h>

> > +#include <linux/of_clk.h>

> > +#include <linux/of_pci.h>

> > +#include <linux/of_platform.h>

> > +#include <linux/pci.h>

> > +#include <linux/phy/phy.h>

> > +#include <linux/platform_device.h>

> > +#include <linux/pm_domain.h>

> > +#include <linux/pm_runtime.h>

> > +#include <linux/reset.h>

> > +

> > +#include "../pci.h"

> > +

> > +#define PCIE_SETTING_REG		0x80

> > +#define PCIE_PCI_IDS_1			0x9c

> > +#define PCI_CLASS(class)		(class << 8)

> > +#define PCIE_RC_MODE			BIT(0)

> > +

> > +#define PCIE_CFGNUM_REG			0x140

> > +#define PCIE_CFG_DEVFN(devfn)		((devfn) & GENMASK(7, 0))

> > +#define PCIE_CFG_BUS(bus)		(((bus) << 8) & GENMASK(15, 8))

> > +#define PCIE_CFG_BYTE_EN(bytes)		(((bytes) << 16) & GENMASK(19, 16))

> > +#define PCIE_CFG_FORCE_BYTE_EN		BIT(20)

> > +#define PCIE_CFG_OFFSET_ADDR		0x1000

> > +#define PCIE_CFG_HEADER(devfn, bus) \

> > +	(PCIE_CFG_DEVFN(devfn) | PCIE_CFG_BUS(bus))

> 

> This should be:

> 

>   (PCIE_CFG_BUS(bus) | PCIE_CFG_DEVFN(devfn))

> 

> to match the order of the bits in the address you're constructing.

> 

> > +#define PCIE_CFG_HEADER_FORCE_BE(devfn, bus, bytes) \

> > +	(PCIE_CFG_HEADER(devfn, bus) | PCIE_CFG_BYTE_EN(bytes) \

> > +	 | PCIE_CFG_FORCE_BYTE_EN)

> 

> Reorder similarly.

> 

> > +#define PCIE_RST_CTRL_REG		0x148

> > +#define PCIE_MAC_RSTB			BIT(0)

> > +#define PCIE_PHY_RSTB			BIT(1)

> > +#define PCIE_BRG_RSTB			BIT(2)

> > +#define PCIE_PE_RSTB			BIT(3)

> > +

> > +#define PCIE_MISC_STATUS_REG		0x14C

> > +#define PCIE_LTR_MSG_RECEIVED		BIT(0)

> > +#define PCIE_PCIE_MSG_RECEIVED		BIT(1)

> 

> Unused, please remove.

> 

> > +#define PCIE_LTSSM_STATUS_REG		0x150

> > +#define PCIE_LTSSM_STATE_MASK		GENMASK(28, 24)

> > +#define PCIE_LTSSM_STATE(val)		((val & PCIE_LTSSM_STATE_MASK) >> 24)

> 

> > +#define PCIE_LTSSM_STATE_L0		0x10

> > +#define PCIE_LTSSM_STATE_L1_IDLE	0x13

> 

> Unused, please remove.

> 

> > +#define PCIE_LTSSM_STATE_L2_IDLE	0x14

> > +

> > +#define PCIE_LINK_STATUS_REG		0x154

> > +#define PCIE_PORT_LINKUP		BIT(8)

> > +

> > +#define PCIE_MSI_SET_NUM		8

> > +#define PCIE_MSI_IRQS_PER_SET		32

> > +#define PCIE_MSI_IRQS_NUM \

> > +	(PCIE_MSI_IRQS_PER_SET * (PCIE_MSI_SET_NUM))

> > +

> > +#define PCIE_INT_ENABLE_REG		0x180

> > +#define PCIE_MSI_MASK			GENMASK(PCIE_MSI_SET_NUM + 8 - 1, 8)

> > +#define PCIE_MSI_SHIFT			8

> > +#define PCIE_INTX_SHIFT			24

> > +#define PCIE_INTX_MASK			GENMASK(27, 24)

> 

> > +#define PCIE_MSG_MASK			BIT(28)

> > +#define PCIE_AER_MASK			BIT(29)

> > +#define PCIE_PM_MASK			BIT(30)

> 

> Unused, please remove.

> 

> > +#define PCIE_INT_STATUS_REG		0x184

> > +#define PCIE_MSI_SET_ENABLE_REG		0x190

> > +

> > +#define PCIE_LOW_POWER_CTRL_REG		0x194

> > +#define PCIE_DIS_LOWPWR_MASK		GENMASK(3, 0)

> > +#define PCIE_DIS_L0S_MASK		BIT(0)

> > +#define PCIE_DIS_L1_MASK		BIT(1)

> > +#define PCIE_DIS_L11_MASK		BIT(2)

> > +#define PCIE_DIS_L12_MASK		BIT(3)

> > +#define PCIE_FORCE_DIS_LOWPWR		GENMASK(11, 8)

> > +#define PCIE_FORCE_DIS_L0S		BIT(8)

> > +#define PCIE_FORCE_DIS_L1		BIT(9)

> > +#define PCIE_FORCE_DIS_L11		BIT(10)

> > +#define PCIE_FORCE_DIS_L12		BIT(11)

> 

> Unused, please remove.

> 

> > +#define PCIE_ICMD_PM_REG		0x198

> > +#define PCIE_TURN_OFF_LINK		BIT(4)

> > +

> > +#define PCIE_MSI_ADDR_BASE_REG		0xc00

> > +#define PCIE_MSI_SET_OFFSET		0x10

> > +#define PCIE_MSI_STATUS_OFFSET		0x04

> > +#define PCIE_MSI_ENABLE_OFFSET		0x08

> > +

> > +#define PCIE_TRANS_TABLE_BASE_REG	0x800

> > +#define PCIE_ATR_SRC_ADDR_MSB_OFFSET	0x4

> > +#define PCIE_ATR_TRSL_ADDR_LSB_OFFSET	0x8

> > +#define PCIE_ATR_TRSL_ADDR_MSB_OFFSET	0xc

> > +#define PCIE_ATR_TRSL_PARAM_OFFSET	0x10

> > +#define PCIE_ATR_TLB_SET_OFFSET		0x20

> > +

> > +#define PCIE_MAX_TRANS_TABLES		8

> > +#define ATR_EN				BIT(0)

> > +#define ATR_SIZE(size)			((((size) - 1) << 1) & GENMASK(6, 1))

> > +#define ATR_ID(id)			(id & GENMASK(3, 0))

> > +#define ATR_PARAM(param)		(((param) << 16) & GENMASK(27, 16))

> > +

> > +/**

> > + * struct mtk_pcie_msi - MSI information for each set

> > + * @base: IO mapped register base

> > + * @irq: MSI set Interrupt number

> > + * @index: MSI set number

> > + * @msg_addr: MSI message address

> > + * @domain: IRQ domain

> > + */

> > +struct mtk_pcie_msi {

> > +	void __iomem *base;

> > +	unsigned int irq;

> > +	int index;

> > +	phys_addr_t msg_addr;

> > +	struct irq_domain *domain;

> > +};

> > +

> > +/**

> > + * struct mtk_pcie_port - PCIe port information

> > + * @dev: PCIe device

> > + * @base: IO mapped register base

> > + * @reg_base: Physical register base

> > + * @mac_reset: mac reset control

> > + * @phy_reset: phy reset control

> > + * @phy: PHY controller block

> > + * @clks: PCIe clocks

> > + * @num_clks: PCIe clocks count for this port

> > + * @is_suspended: device suspend state

> > + * @irq: PCIe controller interrupt number

> > + * @intx_domain: legacy INTx IRQ domain

> > + * @msi_domain: MSI IRQ domain

> > + * @msi_top_domain: MSI IRQ top domain

> > + * @msi_info: MSI sets information

> > + * @lock: lock protecting IRQ bit map

> > + * @msi_irq_in_use: bit map for assigned MSI IRQ

> > + */

> > +struct mtk_pcie_port {

> > +	struct device *dev;

> > +	void __iomem *base;

> > +	phys_addr_t reg_base;

> > +	struct reset_control *mac_reset;

> > +	struct reset_control *phy_reset;

> > +	struct phy *phy;

> > +	struct clk_bulk_data *clks;

> > +	int num_clks;

> > +	bool is_suspended;

> > +

> > +	int irq;

> > +	struct irq_domain *intx_domain;

> > +	struct irq_domain *msi_domain;

> > +	struct irq_domain *msi_top_domain;

> > +	struct mtk_pcie_msi **msi_info;

> > +	struct mutex lock;

> > +	DECLARE_BITMAP(msi_irq_in_use, PCIE_MSI_IRQS_NUM);

> > +};

> > +

> > +static int mtk_pcie_config_read(struct pci_bus *bus, unsigned int devfn,

> > +				    int where, int size, u32 *val)

> > +{

> > +	struct mtk_pcie_port *port = bus->sysdata;

> > +	int bytes;

> > +

> > +	bytes = ((1 << size) - 1) << (where & 0x3);

> 

> This seems like some unusual bit twiddling; at least, I don't remember

> seeing this before.  Can you skim other drivers and see if others do

> the same thing, and adopt a common style if they do?


Hi Bjorn,

Thanks for your review, I will fix it in the next version.
> 

> > +	writel(PCIE_CFG_HEADER_FORCE_BE(devfn, bus->number, bytes),

> > +	       port->base + PCIE_CFGNUM_REG);

> > +

> > +	*val = readl(port->base + PCIE_CFG_OFFSET_ADDR + (where & ~0x3));

> 

> These look like they need to be atomic, since you need a writel()

> followed by a readl().

> 

> pci_lock_config() (used in pci_bus_read_config_*(), etc) uses the

> global pci_lock for this unless CONFIG_PCI_LOCKLESS_CONFIG is set.

> 

> But I would like to eventually move away from this implicit dependency

> on pci_lock.  If you need to make this atomic, can you add the

> explicit locking here, so there's a clear connection between the lock

> and the things it protects?


Sure, I will split it to a map_bus() function and use the standard
pci_generic_config_read32/write32 functions as Rob's suggestion. I think
the potential risks of atomic read/write can be avoided.
> 

> > +	if (size <= 2)

> > +		*val = (*val >> (8 * (where & 0x3))) & ((1 << (size * 8)) - 1);

> > +

> > +	return PCIBIOS_SUCCESSFUL;

> > +}

> > +

> > +static int mtk_pcie_config_write(struct pci_bus *bus, unsigned int devfn,

> > +				     int where, int size, u32 val)

> > +{

> > +	struct mtk_pcie_port *port = bus->sysdata;

> > +	int bytes;

> > +

> > +	bytes = ((1 << size) - 1) << (where & 0x3);

> > +	writel(PCIE_CFG_HEADER_FORCE_BE(devfn, bus->number, bytes),

> > +	       port->base + PCIE_CFGNUM_REG);

> > +

> > +	if (size <= 2)

> > +		val = (val & ((1 << (size * 8)) - 1)) << ((where & 0x3) * 8);

> > +

> > +	writel(val, port->base + PCIE_CFG_OFFSET_ADDR + (where & ~0x3));

> > +

> > +	return PCIBIOS_SUCCESSFUL;

> > +}

> > +

> > +static struct pci_ops mtk_pcie_ops = {

> > +	.read  = mtk_pcie_config_read,

> > +	.write = mtk_pcie_config_write,

> > +};

> > +

> > +static void mtk_pcie_set_trans_window(void __iomem *reg,

> > +				      resource_size_t cpu_addr,

> > +				      resource_size_t pci_addr, size_t size)

> > +{

> > +	writel(lower_32_bits(cpu_addr) | ATR_SIZE(fls(size) - 1) | ATR_EN, reg);

> > +	writel(upper_32_bits(cpu_addr), reg + PCIE_ATR_SRC_ADDR_MSB_OFFSET);

> > +	writel(lower_32_bits(pci_addr), reg + PCIE_ATR_TRSL_ADDR_LSB_OFFSET);

> > +	writel(upper_32_bits(pci_addr), reg + PCIE_ATR_TRSL_ADDR_MSB_OFFSET);

> > +	writel(ATR_ID(0) | ATR_PARAM(0), reg + PCIE_ATR_TRSL_PARAM_OFFSET);

> > +}

> > +

> > +static int mtk_pcie_set_trans_table(void __iomem *reg,

> > +				    resource_size_t cpu_addr,

> > +				    resource_size_t pci_addr, size_t size,

> > +				    int num)

> > +{

> > +	void __iomem *table_base;

> > +

> > +	if (num > PCIE_MAX_TRANS_TABLES)

> > +		return -ENODEV;

> > +

> > +	table_base = reg + num * PCIE_ATR_TLB_SET_OFFSET;

> > +	mtk_pcie_set_trans_window(table_base, cpu_addr, pci_addr, size);

> > +

> > +	return 0;

> > +}

> > +

> > +static int mtk_pcie_startup_port(struct mtk_pcie_port *port)

> > +{

> > +	struct resource_entry *entry;

> > +	struct pci_host_bridge *host = pci_host_bridge_from_priv(port);

> > +	unsigned int table_index = 0;

> > +	int err;

> > +	u32 val;

> > +

> > +	/* Set as RC mode */

> > +	val = readl(port->base + PCIE_SETTING_REG);

> > +	val |= PCIE_RC_MODE;

> > +	writel(val, port->base + PCIE_SETTING_REG);

> > +

> > +	/* Set class code */

> > +	val = readl(port->base + PCIE_PCI_IDS_1);

> > +	val &= ~GENMASK(31, 8);

> > +	val |= PCI_CLASS(PCI_CLASS_BRIDGE_PCI << 8);

> > +	writel(val, port->base + PCIE_PCI_IDS_1);

> > +

> > +	/* Assert all reset signals */

> > +	val = readl(port->base + PCIE_RST_CTRL_REG);

> > +	val |= PCIE_MAC_RSTB | PCIE_PHY_RSTB | PCIE_BRG_RSTB | PCIE_PE_RSTB;

> > +	writel(val, port->base + PCIE_RST_CTRL_REG);

> > +

> > +	/* De-assert reset signals*/

> 

> Missing space at end of comment.

> 

> > +	val &= ~(PCIE_MAC_RSTB | PCIE_PHY_RSTB | PCIE_BRG_RSTB);

> > +	writel(val, port->base + PCIE_RST_CTRL_REG);

> > +

> > +	/* Delay 100ms to wait the reference clocks become stable */

> > +	usleep_range(100 * 1000, 120 * 1000);

> > +

> > +	/* De-assert pe reset*/

> 

> Missing space at end of comment.  "pe" looks like something that

> should be capitalized, since I assume it's some sort of initialism.

> 

> > +	val &= ~PCIE_PE_RSTB;

> > +	writel(val, port->base + PCIE_RST_CTRL_REG);

> > +

> > +	/* Check if the link is up or not */

> > +	err = readl_poll_timeout(port->base + PCIE_LINK_STATUS_REG, val,

> > +			!!(val & PCIE_PORT_LINKUP), 20,

> > +			50 * USEC_PER_MSEC);

> > +	if (err) {

> > +		val = readl(port->base + PCIE_LTSSM_STATUS_REG);

> > +		dev_dbg(port->dev, "ltssm reg val: %#x\n", val);

> > +		return err;

> > +	}

> > +

> > +	/* Set PCIe translation windows */

> > +	resource_list_for_each_entry(entry, &host->windows) {

> > +		unsigned long type = resource_type(entry->res);

> > +		struct resource *res = NULL;

> > +		resource_size_t cpu_addr;

> > +		resource_size_t pci_addr;

> > +

> > +		if (!(type & (IORESOURCE_MEM | IORESOURCE_IO)))

> > +			continue;

> > +

> > +		res = entry->res;

> > +		cpu_addr = res->start;

> > +		pci_addr = res->start - entry->offset;

> > +		mtk_pcie_set_trans_table(port->base + PCIE_TRANS_TABLE_BASE_REG,

> > +					 cpu_addr, pci_addr, resource_size(res),

> > +					 table_index);

> > +

> > +		dev_dbg(port->dev, "Set %s trans window[%d]: cpu_addr = %#llx, pci_addr = %#llx, size = %#llx\n",

> > +			(!!(type & IORESOURCE_MEM) ? "MEM" : "IO"), table_index,

> > +			cpu_addr, pci_addr, resource_size(res));

> > +

> > +		table_index++;

> > +	}

> > +

> > +	return 0;

> > +}

> > +

> > +static inline struct mtk_pcie_msi *mtk_get_msi_info(struct mtk_pcie_port *port,

> > +						    unsigned long hwirq)

> > +{

> > +	return port->msi_info[hwirq / PCIE_MSI_IRQS_PER_SET];

> > +}

> > +

> > +static int mtk_pcie_set_affinity(struct irq_data *data,

> > +				 const struct cpumask *mask, bool force)

> > +{

> > +	struct mtk_pcie_port *port = irq_data_get_irq_chip_data(data);

> > +	int ret;

> > +

> > +	ret = irq_set_affinity_hint(port->irq, mask);

> > +	if (ret)

> > +		return ret;

> > +

> > +	irq_data_update_effective_affinity(data, mask);

> > +

> > +	return 0;

> > +}

> > +

> > +static void mtk_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)

> > +{

> > +	struct mtk_pcie_msi *msi_info;

> > +	struct mtk_pcie_port *port = irq_data_get_irq_chip_data(data);

> > +

> > +	msi_info = mtk_get_msi_info(port, data->hwirq);

> > +

> > +	msg->address_hi = 0;

> > +	msg->address_lo = lower_32_bits(msi_info->msg_addr);

> > +

> > +	msg->data = data->hwirq;

> > +	dev_dbg(port->dev, "msi#%#lx address_hi %#x address_lo %#x data %d\n",

> > +		data->hwirq, msg->address_hi, msg->address_lo, msg->data);

> > +}

> > +

> > +static void mtk_msi_irq_ack(struct irq_data *data)

> > +{

> > +	struct mtk_pcie_msi *msi_info;

> > +	struct mtk_pcie_port *port = irq_data_get_irq_chip_data(data);

> > +	unsigned long hwirq;

> > +

> > +	msi_info = mtk_get_msi_info(port, data->hwirq);

> > +

> > +	hwirq =	data->hwirq % PCIE_MSI_IRQS_PER_SET;

> > +

> > +	writel(1 << hwirq, msi_info->base + PCIE_MSI_STATUS_OFFSET);

> > +}

> > +

> > +static void mtk_msi_irq_mask(struct irq_data *data)

> > +{

> > +	struct mtk_pcie_msi *msi_info;

> > +	struct mtk_pcie_port *port = irq_data_get_irq_chip_data(data);

> > +	u32 val;

> > +

> > +	msi_info = mtk_get_msi_info(port, data->hwirq);

> > +	val = readl(msi_info->base + PCIE_MSI_ENABLE_OFFSET);

> > +	val &= ~(1 << data->hwirq);

> > +	writel(val, msi_info->base + PCIE_MSI_ENABLE_OFFSET);

> > +

> > +	pci_msi_mask_irq(data);

> > +}

> > +

> > +static void mtk_msi_irq_unmask(struct irq_data *data)

> > +{

> > +	struct mtk_pcie_msi *msi_info;

> > +	struct mtk_pcie_port *port = irq_data_get_irq_chip_data(data);

> > +	u32 val;

> > +

> > +	msi_info = mtk_get_msi_info(port, data->hwirq);

> > +	val = readl(msi_info->base + PCIE_MSI_ENABLE_OFFSET);

> > +	val |= (1 << data->hwirq);

> > +	writel(val, msi_info->base + PCIE_MSI_ENABLE_OFFSET);

> > +

> > +	pci_msi_unmask_irq(data);

> > +}

> > +

> > +static struct irq_chip mtk_msi_irq_chip = {

> > +	.irq_ack		= mtk_msi_irq_ack,

> > +	.irq_compose_msi_msg	= mtk_compose_msi_msg,

> > +	.irq_mask		= mtk_msi_irq_mask,

> > +	.irq_unmask		= mtk_msi_irq_unmask,

> > +	.irq_set_affinity	= mtk_pcie_set_affinity,

> > +	.name			= "PCIe",

> > +};

> > +

> > +static irq_hw_number_t mtk_pcie_msi_get_hwirq(struct msi_domain_info *info,

> > +					      msi_alloc_info_t *arg)

> > +{

> > +	struct msi_desc *entry = arg->desc;

> > +	struct mtk_pcie_port *port = info->chip_data;

> > +	int hwirq;

> > +

> > +	mutex_lock(&port->lock);

> > +

> > +	hwirq = bitmap_find_free_region(port->msi_irq_in_use, PCIE_MSI_IRQS_NUM,

> > +			order_base_2(entry->nvec_used));

> > +	if (hwirq < 0) {

> > +		mutex_unlock(&port->lock);

> > +		return -ENOSPC;

> > +	}

> > +

> > +	mutex_unlock(&port->lock);

> > +

> > +	return hwirq;

> > +}

> > +

> > +static void mtk_pcie_msi_free(struct irq_domain *domain,

> > +			      struct msi_domain_info *info, unsigned int virq)

> > +{

> > +	struct irq_data *data = irq_domain_get_irq_data(domain, virq);

> > +	struct mtk_pcie_port *port = irq_data_get_irq_chip_data(data);

> > +

> > +	mutex_lock(&port->lock);

> > +

> > +	bitmap_clear(port->msi_irq_in_use, data->hwirq, 1);

> > +

> > +	mutex_unlock(&port->lock);

> > +}

> > +

> > +static struct msi_domain_ops mtk_msi_domain_ops = {

> > +	.get_hwirq	= mtk_pcie_msi_get_hwirq,

> > +	.msi_free	= mtk_pcie_msi_free,

> > +};

> > +

> > +static struct msi_domain_info mtk_msi_domain_info = {

> > +	.flags		= (MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_PCI_MSIX |

> > +			   MSI_FLAG_USE_DEF_CHIP_OPS | MSI_FLAG_MULTI_PCI_MSI),

> > +	.chip		= &mtk_msi_irq_chip,

> > +	.ops		= &mtk_msi_domain_ops,

> > +	.handler	= handle_edge_irq,

> > +	.handler_name	= "MSI",

> > +};

> > +

> > +static void mtk_msi_top_irq_eoi(struct irq_data *data)

> > +{

> > +	struct mtk_pcie_port *port = irq_data_get_irq_chip_data(data);

> > +	unsigned long msi_irq = data->hwirq + PCIE_MSI_SHIFT;

> > +

> > +	writel(1 << msi_irq, port->base + PCIE_INT_STATUS_REG);

> > +}

> > +

> > +static struct irq_chip mtk_msi_top_irq_chip = {

> > +	.irq_eoi	= mtk_msi_top_irq_eoi,

> > +	.name		= "PCIe",

> > +};

> > +

> > +static void mtk_pcie_msi_handler(struct irq_desc *desc)

> > +{

> > +	struct mtk_pcie_msi *msi_info = irq_desc_get_handler_data(desc);

> > +	struct irq_chip *irqchip = irq_desc_get_chip(desc);

> > +	unsigned long msi_enable, msi_status;

> > +	unsigned int virq;

> > +	irq_hw_number_t bit, hwirq;

> > +

> > +	chained_irq_enter(irqchip, desc);

> > +

> > +	msi_enable = readl(msi_info->base + PCIE_MSI_ENABLE_OFFSET);

> > +	while ((msi_status = readl(msi_info->base + PCIE_MSI_STATUS_OFFSET))) {

> > +		msi_status &= msi_enable;

> > +		for_each_set_bit(bit, &msi_status, PCIE_MSI_IRQS_PER_SET) {

> > +			hwirq = bit + msi_info->index * PCIE_MSI_IRQS_PER_SET;

> > +			virq = irq_find_mapping(msi_info->domain, hwirq);

> > +			generic_handle_irq(virq);

> > +		}

> > +	}

> > +

> > +	chained_irq_exit(irqchip, desc);

> > +}

> > +

> > +static int mtk_msi_top_domain_map(struct irq_domain *domain,

> > +				    unsigned int virq, irq_hw_number_t hwirq)

> > +{

> > +	struct mtk_pcie_port *port = domain->host_data;

> > +	struct mtk_pcie_msi *msi_info = port->msi_info[hwirq];

> > +

> > +	irq_domain_set_info(domain, virq, hwirq,

> > +			    &mtk_msi_top_irq_chip, domain->host_data,

> > +			    mtk_pcie_msi_handler, msi_info, NULL);

> > +

> > +	return 0;

> > +}

> > +

> > +static const struct irq_domain_ops mtk_msi_top_domain_ops = {

> > +	.map = mtk_msi_top_domain_map,

> > +};

> > +

> > +static void mtk_intx_mask(struct irq_data *data)

> > +{

> > +	struct mtk_pcie_port *port = irq_data_get_irq_chip_data(data);

> > +	u32 val;

> > +

> > +	val = readl(port->base + PCIE_INT_ENABLE_REG);

> > +	val &= ~(1 << (data->hwirq + PCIE_INTX_SHIFT));

> > +	writel(val, port->base + PCIE_INT_ENABLE_REG);

> > +}

> > +

> > +static void mtk_intx_unmask(struct irq_data *data)

> > +{

> > +	struct mtk_pcie_port *port = irq_data_get_irq_chip_data(data);

> > +	u32 val;

> > +

> > +	val = readl(port->base + PCIE_INT_ENABLE_REG);

> > +	val |= 1 << (data->hwirq + PCIE_INTX_SHIFT);

> > +	writel(val, port->base + PCIE_INT_ENABLE_REG);

> > +}

> > +

> > +static void mtk_intx_eoi(struct irq_data *data)

> > +{

> > +	struct mtk_pcie_port *port = irq_data_get_irq_chip_data(data);

> > +	unsigned long hwirq;

> > +

> > +	/**

> > +	 * As an emulated level irq, its interrupt status will be remained

> > +	 * until receive the corresponding message of de-assert, hence that

> > +	 * the status can only be cleared when the interrupt has been serviced.

> 

> s/irq/IRQ/

> 

>   As an emulated level IRQ, its interrupt status will remain until the

>   corresponding de-assert message is received; hence that ...

> 

> > +	 */

> > +	hwirq = data->hwirq + PCIE_INTX_SHIFT;

> > +	writel(1 << hwirq, port->base + PCIE_INT_STATUS_REG);

> > +}

> > +

> > +static struct irq_chip mtk_intx_irq_chip = {

> > +	.irq_mask		= mtk_intx_mask,

> > +	.irq_unmask		= mtk_intx_unmask,

> > +	.irq_eoi		= mtk_intx_eoi,

> > +	.irq_set_affinity	= mtk_pcie_set_affinity,

> > +	.name			= "PCIe",

> > +};

> > +

> > +static int mtk_pcie_intx_map(struct irq_domain *domain, unsigned int irq,

> > +			     irq_hw_number_t hwirq)

> > +{

> > +	irq_set_chip_and_handler_name(irq, &mtk_intx_irq_chip,

> > +				      handle_fasteoi_irq, "INTx");

> > +	irq_set_chip_data(irq, domain->host_data);

> > +

> > +	return 0;

> > +}

> > +

> > +static const struct irq_domain_ops intx_domain_ops = {

> > +	.map = mtk_pcie_intx_map,

> > +};

> > +

> > +static int mtk_pcie_init_irq_domains(struct mtk_pcie_port *port,

> > +				     struct device_node *node)

> > +{

> > +	struct device *dev = port->dev;

> > +	struct device_node *intc_node;

> > +	struct fwnode_handle *fwnode = of_node_to_fwnode(node);

> > +	struct mtk_pcie_msi *msi_info;

> > +	struct msi_domain_info *info;

> > +	int i, ret;

> > +

> > +	/* Setup INTx */

> > +	intc_node = of_get_child_by_name(node, "interrupt-controller");

> > +	if (!intc_node) {

> > +		dev_notice(dev, "Missing PCIe Intc node\n");

> 

> s/Missing/missing/ to match other messages.

> 

> > +		return -ENODEV;

> > +	}

> > +

> > +	port->intx_domain = irq_domain_add_linear(intc_node, PCI_NUM_INTX,

> > +						  &intx_domain_ops, port);

> > +	if (!port->intx_domain) {

> > +		dev_notice(dev, "failed to get INTx IRQ domain\n");

> > +		return -ENODEV;

> > +	}

> > +

> > +	/* Setup MSI */

> > +	mutex_init(&port->lock);

> > +

> > +	info = devm_kzalloc(dev, sizeof(*info), GFP_KERNEL);

> > +	if (!info)

> > +		return -ENOMEM;

> > +

> > +	memcpy(info, &mtk_msi_domain_info, sizeof(*info));

> > +	info->chip_data = port;

> > +

> > +	port->msi_domain = pci_msi_create_irq_domain(fwnode, info, NULL);

> > +	if (!port->msi_domain) {

> > +		dev_info(dev, "failed to create msi domain\n");

> 

> s/msi/MSI/

> 

> > +		ret = -ENODEV;

> > +		goto err_msi_domain;

> > +	}

> > +

> > +	/* Enable MSI and setup pcie domains */

> 

> s/pcie/PCIe/

> 

> > +	port->msi_top_domain = irq_domain_add_hierarchy(NULL, 0, 0, node,

> > +							&mtk_msi_top_domain_ops,

> > +							port);

> > +	if (!port->msi_top_domain) {

> > +		dev_info(dev, "failed to create msi top domain\n");

> 

> s/msi/MSI/

> 

> > +		ret = -ENODEV;

> > +		goto err_msi_top_domain;

> > +	}

> > +

> > +	port->msi_info = devm_kzalloc(dev, PCIE_MSI_SET_NUM, GFP_KERNEL);

> > +	if (!port->msi_info) {

> > +		ret = -ENOMEM;

> > +		goto err_msi_info;

> > +	}

> > +

> > +	for (i = 0; i < PCIE_MSI_SET_NUM; i++) {

> > +		int offset = i * PCIE_MSI_SET_OFFSET;

> > +		u32 val;

> > +

> > +		msi_info = devm_kzalloc(dev, sizeof(*msi_info), GFP_KERNEL);

> > +		if (!msi_info) {

> > +			ret = -ENOMEM;

> > +			goto err_msi_set;

> > +		}

> > +

> > +		msi_info->base = port->base + PCIE_MSI_ADDR_BASE_REG + offset;

> > +		msi_info->msg_addr = port->reg_base + PCIE_MSI_ADDR_BASE_REG +

> > +				     offset;

> > +

> > +		writel(lower_32_bits(msi_info->msg_addr), msi_info->base);

> > +

> > +		msi_info->index = i;

> > +		msi_info->domain = port->msi_domain;

> > +

> > +		port->msi_info[i] = msi_info;

> > +

> > +		/* Alloc irq for each msi set */

> 

> s/irq/IRQ/

> s/msi/MSI/

> 

> > +		msi_info->irq = irq_create_mapping(port->msi_top_domain, i);

> > +		if (!msi_info->irq) {

> > +			dev_info(dev, "allocate msi top irq failed\n");

> 

> s/irq/IRQ/

> s/msi/MSI/

> 

> > +			ret = -ENOSPC;

> > +			goto err_msi_set;

> > +		}

> > +

> > +		val = readl(port->base + PCIE_INT_ENABLE_REG);

> > +		val |= (1 << (i + PCIE_MSI_SHIFT));

> > +		writel(val, port->base + PCIE_INT_ENABLE_REG);

> > +

> > +		val = readl(port->base + PCIE_MSI_SET_ENABLE_REG);

> > +		val |= (1 << i);

> > +		writel(val, port->base + PCIE_MSI_SET_ENABLE_REG);

> > +	}

> > +

> > +	return 0;

> > +

> > +err_msi_set:

> > +	while (--i >= 0) {

> 

> (i-- > 0) would be more idiomatic.

> 

> > +		msi_info = port->msi_info[i];

> > +		irq_dispose_mapping(msi_info->irq);

> > +	}

> > +err_msi_info:

> > +	irq_domain_remove(port->msi_top_domain);

> > +err_msi_top_domain:

> > +	irq_domain_remove(port->msi_domain);

> > +err_msi_domain:

> > +	irq_domain_remove(port->intx_domain);

> > +

> > +	return ret;

> > +}

> > +

> > +static void mtk_pcie_irq_teardown(struct mtk_pcie_port *port)

> > +{

> > +	struct mtk_pcie_msi *msi_info;

> > +	int i;

> > +

> > +	irq_set_chained_handler_and_data(port->irq, NULL, NULL);

> > +

> > +	if (port->intx_domain)

> > +		irq_domain_remove(port->intx_domain);

> > +

> > +	if (port->msi_domain)

> > +		irq_domain_remove(port->msi_domain);

> > +

> > +	if (port->msi_top_domain) {

> > +		for (i = 0; i < PCIE_MSI_SET_NUM; i++) {

> > +			msi_info = port->msi_info[i];

> > +			irq_dispose_mapping(msi_info->irq);

> > +		}

> > +

> > +		irq_domain_remove(port->msi_top_domain);

> > +	}

> > +

> > +	irq_dispose_mapping(port->irq);

> > +}

> > +

> > +static void mtk_pcie_irq_handler(struct irq_desc *desc)

> > +{

> > +	struct mtk_pcie_port *port = irq_desc_get_handler_data(desc);

> > +	struct irq_chip *irqchip = irq_desc_get_chip(desc);

> > +	unsigned long status;

> > +	unsigned int virq;

> > +	irq_hw_number_t irq_bit = PCIE_INTX_SHIFT;

> > +

> > +	chained_irq_enter(irqchip, desc);

> > +

> > +	status = readl(port->base + PCIE_INT_STATUS_REG);

> > +	if (status & PCIE_INTX_MASK) {

> > +		for_each_set_bit_from(irq_bit, &status, PCI_NUM_INTX +

> > +				      PCIE_INTX_SHIFT) {

> > +			virq = irq_find_mapping(port->intx_domain,

> > +						irq_bit - PCIE_INTX_SHIFT);

> > +			generic_handle_irq(virq);

> > +		}

> > +	}

> > +

> > +	if (status & PCIE_MSI_MASK) {

> > +		irq_bit = PCIE_MSI_SHIFT;

> > +		for_each_set_bit_from(irq_bit, &status, PCIE_MSI_SET_NUM +

> > +				      PCIE_MSI_SHIFT) {

> > +			virq = irq_find_mapping(port->msi_top_domain,

> > +						irq_bit - PCIE_MSI_SHIFT);

> > +			generic_handle_irq(virq);

> > +		}

> > +	}

> > +

> > +	chained_irq_exit(irqchip, desc);

> > +}

> > +

> > +static int mtk_pcie_setup_irq(struct mtk_pcie_port *port,

> > +			      struct device_node *node)

> > +{

> > +	struct device *dev = port->dev;

> > +	struct platform_device *pdev = to_platform_device(dev);

> > +	int err;

> > +

> > +	err = mtk_pcie_init_irq_domains(port, node);

> > +	if (err) {

> > +		dev_notice(dev, "failed to init PCIe IRQ domain\n");

> > +		return err;

> > +	}

> > +

> > +	port->irq = platform_get_irq(pdev, 0);

> > +	if (port->irq < 0)

> > +		return port->irq;

> > +

> > +	irq_set_chained_handler_and_data(port->irq, mtk_pcie_irq_handler, port);

> > +

> > +	return 0;

> > +}

> > +

> > +static int mtk_pcie_clk_init(struct mtk_pcie_port *port)

> > +{

> > +	int ret;

> > +

> > +	port->num_clks = devm_clk_bulk_get_all(port->dev, &port->clks);

> > +	if (port->num_clks < 0) {

> > +		dev_notice(port->dev, "failed to get pcie clock\n");

> 

> s/pcie/PCIe/

> 

> > +		return port->num_clks;

> > +	}

> > +

> > +	ret = clk_bulk_prepare_enable(port->num_clks, port->clks);

> > +	if (ret) {

> > +		dev_notice(port->dev, "failed to enable pcie clocks\n");

> 

> s/pcie/PCIe/

> 

> > +		return ret;

> > +	}

> > +

> > +	return 0;

> > +}

> > +

> > +static int mtk_pcie_power_up(struct mtk_pcie_port *port)

> > +{

> > +	struct device *dev = port->dev;

> > +	int err;

> > +

> > +	port->phy_reset = devm_reset_control_get_optional_exclusive(dev, "phy");

> > +	if (IS_ERR(port->phy_reset))

> > +		return PTR_ERR(port->phy_reset);

> > +

> > +	/* PHY power on and enable pipe clock */

> > +	port->phy = devm_phy_optional_get(dev, "pcie-phy");

> > +	if (IS_ERR(port->phy))

> > +		return PTR_ERR(port->phy);

> > +

> > +	reset_control_deassert(port->phy_reset);

> > +

> > +	err = phy_power_on(port->phy);

> > +	if (err) {

> > +		dev_notice(dev, "failed to power on pcie phy\n");

> 

> s/pcie/PCIe/

> 

> > +		goto err_phy_on;

> > +	}

> > +

> > +	err = phy_init(port->phy);

> > +	if (err) {

> > +		dev_notice(dev, "failed to initialize pcie phy\n");

> 

> s/pcie/PCIe/

> 

> > +		goto err_phy_init;

> > +	}

> > +

> > +	port->mac_reset = devm_reset_control_get_optional_exclusive(dev, "mac");

> > +	if (IS_ERR(port->mac_reset)) {

> > +		err = PTR_ERR(port->mac_reset);

> > +		goto err_mac_rst;

> > +	}

> > +

> > +	reset_control_deassert(port->mac_reset);

> > +

> > +	/* MAC power on and enable transaction layer clocks */

> > +	pm_runtime_enable(dev);

> > +	pm_runtime_get_sync(dev);

> > +

> > +	err = mtk_pcie_clk_init(port);

> > +	if (err) {

> > +		dev_notice(dev, "clock init failed\n");

> > +		goto err_clk_init;

> > +	}

> > +

> > +	return 0;

> > +

> > +err_clk_init:

> > +	pm_runtime_put_sync(dev);

> > +	pm_runtime_disable(dev);

> > +	reset_control_assert(port->mac_reset);

> > +err_mac_rst:

> > +	phy_exit(port->phy);

> > +err_phy_init:

> > +	phy_power_off(port->phy);

> > +err_phy_on:

> > +	reset_control_assert(port->phy_reset);

> > +

> > +	return err;

> > +}

> > +

> > +static void mtk_pcie_power_down(struct mtk_pcie_port *port)

> > +{

> > +	clk_bulk_disable_unprepare(port->num_clks, port->clks);

> > +

> > +	pm_runtime_put_sync(port->dev);

> > +	pm_runtime_disable(port->dev);

> > +	reset_control_assert(port->mac_reset);

> > +

> > +	phy_power_off(port->phy);

> > +	phy_exit(port->phy);

> > +	reset_control_assert(port->phy_reset);

> > +}

> > +

> > +static int mtk_pcie_setup(struct mtk_pcie_port *port)

> > +{

> > +	struct device *dev = port->dev;

> > +	struct platform_device *pdev = to_platform_device(dev);

> > +	struct resource *regs;

> > +	int err;

> > +

> > +	regs = platform_get_resource_byname(pdev, IORESOURCE_MEM, "pcie-mac");

> > +	port->base = devm_ioremap_resource(dev, regs);

> > +	if (IS_ERR(port->base)) {

> > +		dev_notice(dev, "failed to map register base\n");

> > +		return PTR_ERR(port->base);

> > +	}

> > +

> > +	port->reg_base = regs->start;

> > +

> > +	/* Don't touch the hardware registers before power up */

> > +	err = mtk_pcie_power_up(port);

> > +	if (err)

> > +		return err;

> > +

> > +	/* Try link up */

> > +	err = mtk_pcie_startup_port(port);

> > +	if (err) {

> > +		dev_notice(dev, "PCIe link down\n");

> > +		goto err_setup;

> 

> Generally it should not be a fatal error if the link is not up at

> probe-time.  You may be able to hot-add a device, or the device may

> have some external power control that will power it up later.


This is for the power saving requirement. If there is no device
connected with the PCIe slot, the PCIe MAC and PHY should be powered
off.

Is there any standard flow to support power down the hardware at
probe-time if no device is connected and power it up when hot-add a
device?

Thanks.
 
> 

> > +	}

> > +

> > +	err = mtk_pcie_setup_irq(port, dev->of_node);

> > +	if (err)

> > +		goto err_setup;

> > +

> > +	dev_info(dev, "PCIe link up success!\n");

> > +

> > +	return 0;

> > +

> > +err_setup:

> > +	mtk_pcie_power_down(port);

> > +

> > +	return err;

> > +}

> > +

> > +static int mtk_pcie_probe(struct platform_device *pdev)

> > +{

> > +	struct device *dev = &pdev->dev;

> > +	struct mtk_pcie_port *port;

> > +	struct pci_host_bridge *host;

> > +	int err;

> > +

> > +	host = devm_pci_alloc_host_bridge(dev, sizeof(*port));

> > +	if (!host)

> > +		return -ENOMEM;

> > +

> > +	port = pci_host_bridge_priv(host);

> > +

> > +	port->dev = dev;

> > +	platform_set_drvdata(pdev, port);

> > +

> > +	err = mtk_pcie_setup(port);

> > +	if (err)

> > +		return err;

> > +

> > +	host->ops = &mtk_pcie_ops;

> > +	host->sysdata = port;

> > +

> > +	err = pci_host_probe(host);

> > +	if (err) {

> > +		mtk_pcie_power_down(port);

> > +		return err;

> > +	}

> > +

> > +	return 0;

> > +}

> > +

> > +static int mtk_pcie_remove(struct platform_device *pdev)

> > +{

> > +	struct mtk_pcie_port *port = platform_get_drvdata(pdev);

> > +	struct pci_host_bridge *host = pci_host_bridge_from_priv(port);

> > +

> > +	pci_lock_rescan_remove();

> > +	pci_stop_root_bus(host->bus);

> > +	pci_remove_root_bus(host->bus);

> > +	pci_unlock_rescan_remove();

> > +

> > +	mtk_pcie_irq_teardown(port);

> > +	mtk_pcie_power_down(port);

> > +

> > +	return 0;

> > +}

> > +

> > +static int __maybe_unused mtk_pcie_turn_off_link(struct mtk_pcie_port *port)

> > +{

> > +	u32 val;

> > +

> > +	val = readl(port->base + PCIE_ICMD_PM_REG);

> > +	val |= PCIE_TURN_OFF_LINK;

> > +	writel(val, port->base + PCIE_ICMD_PM_REG);

> > +

> > +	/* Check the link is L2 */

> > +	return readl_poll_timeout(port->base + PCIE_LTSSM_STATUS_REG, val,

> > +				  (PCIE_LTSSM_STATE(val) ==

> > +				   PCIE_LTSSM_STATE_L2_IDLE), 20,

> > +				   50 * USEC_PER_MSEC);

> > +}

> > +

> > +static int __maybe_unused mtk_pcie_suspend_noirq(struct device *dev)

> > +{

> > +	struct mtk_pcie_port *port = dev_get_drvdata(dev);

> > +	int err;

> > +	u32 val;

> > +

> > +	if (port->is_suspended)

> > +		return 0;

> > +

> > +	/* Trigger link to L2 state */

> > +	err = mtk_pcie_turn_off_link(port);

> > +	if (err) {

> > +		dev_notice(port->dev, "can not enter L2 state\n");

> > +		goto power_off;

> > +	}

> > +

> > +	/* Pull down the PERST# pin */

> > +	val = readl(port->base + PCIE_RST_CTRL_REG);

> > +	val |= PCIE_PE_RSTB;

> > +	writel(val, port->base + PCIE_RST_CTRL_REG);

> > +

> > +	dev_info(port->dev, "enter L2 state success");

> > +

> > +power_off:

> > +	clk_bulk_disable_unprepare(port->num_clks, port->clks);

> > +

> > +	phy_power_off(port->phy);

> > +

> > +	port->is_suspended = true;

> > +

> > +	return 0;

> > +}

> > +

> > +static int __maybe_unused mtk_pcie_resume_noirq(struct device *dev)

> > +{

> > +	struct mtk_pcie_port *port = dev_get_drvdata(dev);

> > +	int err;

> > +

> > +	if (!port->is_suspended)

> > +		return 0;

> > +

> > +	phy_power_on(port->phy);

> > +

> > +	err = clk_bulk_prepare_enable(port->num_clks, port->clks);

> > +	if (err) {

> > +		dev_dbg(dev, "failed to enable pcie clocks\n");

> 

> s/pcie/PCIe/

> 

> > +		return err;

> > +	}

> > +

> > +	err = mtk_pcie_startup_port(port);

> > +	if (err) {

> > +		dev_notice(port->dev, "resume failed\n");

> > +		return err;

> > +	}

> > +

> > +	port->is_suspended = false;

> > +

> > +	dev_info(port->dev, "resume done\n");

> > +

> > +	return 0;

> > +}

> > +

> > +static const struct dev_pm_ops mtk_pcie_pm_ops = {

> > +	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(mtk_pcie_suspend_noirq,

> > +				      mtk_pcie_resume_noirq)

> > +};

> > +

> > +static const struct of_device_id mtk_pcie_of_match[] = {

> > +	{ .compatible = "mediatek,mt8192-pcie" },

> > +	{},

> > +};

> > +

> > +static struct platform_driver mtk_pcie_driver = {

> > +	.probe = mtk_pcie_probe,

> > +	.remove = mtk_pcie_remove,

> > +	.driver = {

> > +		.name = "mtk-pcie",

> > +		.of_match_table = mtk_pcie_of_match,

> > +		.pm = &mtk_pcie_pm_ops,

> > +	},

> > +};

> > +

> > +module_platform_driver(mtk_pcie_driver);

> > +MODULE_LICENSE("GPL v2");

> > -- 

> > 2.25.1

> >

On Sun, Nov 22, 2020 at 11:45 PM Jianjun Wang <jianjun.wang@mediatek.com> wrote:
>

> On Thu, 2020-11-19 at 14:28 -0600, Bjorn Helgaas wrote:

> > "Add new generation" really contains no information.  And "mediatek"

> > is already used for the pcie-mediatek.c driver, so we should have a

> > new tag for this new driver.  Include useful information in the

> > subject, e.g.,

> >

> >   PCI: mediatek-gen3: Add MediaTek Gen3 driver for MT8192

> >

> > On Wed, Nov 18, 2020 at 04:29:34PM +0800, Jianjun Wang wrote:

> > > MediaTek's PCIe host controller has three generation HWs, the new

> > > generation HW is an individual bridge, it supoorts Gen3 speed and

> > > up to 256 MSI interrupt numbers for multi-function devices.

> >

> > s/supoorts/supports/

> >

> > > Add support for new Gen3 controller which can be found on MT8192.

> > >

> > > Signed-off-by: Jianjun Wang <jianjun.wang@mediatek.com>

> > > Acked-by: Ryder Lee <ryder.lee@mediatek.com>


[...]

> > > +static int mtk_pcie_config_read(struct pci_bus *bus, unsigned int devfn,

> > > +                               int where, int size, u32 *val)

> > > +{

> > > +   struct mtk_pcie_port *port = bus->sysdata;

> > > +   int bytes;

> > > +

> > > +   bytes = ((1 << size) - 1) << (where & 0x3);

> >

> > This seems like some unusual bit twiddling; at least, I don't remember

> > seeing this before.  Can you skim other drivers and see if others do

> > the same thing, and adopt a common style if they do?

>

> Hi Bjorn,

>

> Thanks for your review, I will fix it in the next version.

> >

> > > +   writel(PCIE_CFG_HEADER_FORCE_BE(devfn, bus->number, bytes),

> > > +          port->base + PCIE_CFGNUM_REG);

> > > +

> > > +   *val = readl(port->base + PCIE_CFG_OFFSET_ADDR + (where & ~0x3));

> >

> > These look like they need to be atomic, since you need a writel()

> > followed by a readl().

> >

> > pci_lock_config() (used in pci_bus_read_config_*(), etc) uses the

> > global pci_lock for this unless CONFIG_PCI_LOCKLESS_CONFIG is set.

> >

> > But I would like to eventually move away from this implicit dependency

> > on pci_lock.  If you need to make this atomic, can you add the

> > explicit locking here, so there's a clear connection between the lock

> > and the things it protects?

>

> Sure, I will split it to a map_bus() function and use the standard

> pci_generic_config_read32/write32 functions as Rob's suggestion. I think

> the potential risks of atomic read/write can be avoided.


The generic functions have no effect on atomicity, but using them does
make it easier to find the non-atomic cases.

I'm not sure that having host drivers do their own locking is the best
approach. That's a recipe for more cleanups. It's a common enough
issue that I think it's better if we have locking done in 1 place.
Then host drivers can simply say if they need locking or not via some
bus flag.

Rob

[+cc Lukas, pciehp power control question]

On Mon, Nov 23, 2020 at 02:45:13PM +0800, Jianjun Wang wrote:
> On Thu, 2020-11-19 at 14:28 -0600, Bjorn Helgaas wrote:

> > "Add new generation" really contains no information.  And "mediatek"

> > is already used for the pcie-mediatek.c driver, so we should have a

> > new tag for this new driver.  Include useful information in the

> > subject, e.g.,

> > 

> >   PCI: mediatek-gen3: Add MediaTek Gen3 driver for MT8192


> > > +static int mtk_pcie_setup(struct mtk_pcie_port *port)

> > > +{

> > > +	struct device *dev = port->dev;

> > > +	struct platform_device *pdev = to_platform_device(dev);

> > > +	struct resource *regs;

> > > +	int err;

> > > +

> > > +	regs = platform_get_resource_byname(pdev, IORESOURCE_MEM, "pcie-mac");

> > > +	port->base = devm_ioremap_resource(dev, regs);

> > > +	if (IS_ERR(port->base)) {

> > > +		dev_notice(dev, "failed to map register base\n");

> > > +		return PTR_ERR(port->base);

> > > +	}

> > > +

> > > +	port->reg_base = regs->start;

> > > +

> > > +	/* Don't touch the hardware registers before power up */

> > > +	err = mtk_pcie_power_up(port);

> > > +	if (err)

> > > +		return err;

> > > +

> > > +	/* Try link up */

> > > +	err = mtk_pcie_startup_port(port);

> > > +	if (err) {

> > > +		dev_notice(dev, "PCIe link down\n");

> > > +		goto err_setup;

> > 

> > Generally it should not be a fatal error if the link is not up at

> > probe-time.  You may be able to hot-add a device, or the device may

> > have some external power control that will power it up later.

> 

> This is for the power saving requirement. If there is no device

> connected with the PCIe slot, the PCIe MAC and PHY should be powered

> off.

> 

> Is there any standard flow to support power down the hardware at

> probe-time if no device is connected and power it up when hot-add a

> device?


That's a good question.  I assume this looks like a standard PCIe
hot-add event?

When you hot-add a device, does the Root Port generate a Presence
Detect Changed interrupt?  The pciehp driver should field that
interrupt and turn on power to the slot via the Power Controller
Control bit in the Slot Control register.

Does your hardware require something more than that to control the MAC
and PHY power?

Bjorn

On Mon, Nov 30, 2020 at 09:05:48AM -0700, Rob Herring wrote:
> On Sun, Nov 22, 2020 at 11:45 PM Jianjun Wang <jianjun.wang@mediatek.com> wrote:

> > On Thu, 2020-11-19 at 14:28 -0600, Bjorn Helgaas wrote:

> > > "Add new generation" really contains no information.  And "mediatek"

> > > is already used for the pcie-mediatek.c driver, so we should have a

> > > new tag for this new driver.  Include useful information in the

> > > subject, e.g.,

> > >

> > >   PCI: mediatek-gen3: Add MediaTek Gen3 driver for MT8192


> > > > +   writel(PCIE_CFG_HEADER_FORCE_BE(devfn, bus->number, bytes),

> > > > +          port->base + PCIE_CFGNUM_REG);

> > > > +

> > > > +   *val = readl(port->base + PCIE_CFG_OFFSET_ADDR + (where & ~0x3));

> > >

> > > These look like they need to be atomic, since you need a writel()

> > > followed by a readl().

> > >

> > > pci_lock_config() (used in pci_bus_read_config_*(), etc) uses the

> > > global pci_lock for this unless CONFIG_PCI_LOCKLESS_CONFIG is set.

> > >

> > > But I would like to eventually move away from this implicit dependency

> > > on pci_lock.  If you need to make this atomic, can you add the

> > > explicit locking here, so there's a clear connection between the lock

> > > and the things it protects?

> >

> > Sure, I will split it to a map_bus() function and use the standard

> > pci_generic_config_read32/write32 functions as Rob's suggestion. I think

> > the potential risks of atomic read/write can be avoided.

> 

> The generic functions have no effect on atomicity, but using them does

> make it easier to find the non-atomic cases.

> 

> I'm not sure that having host drivers do their own locking is the best

> approach. That's a recipe for more cleanups. It's a common enough

> issue that I think it's better if we have locking done in 1 place.

> Then host drivers can simply say if they need locking or not via some

> bus flag.


Yeah, you may be right.  I guess we don't have to make it an issue for
this patch; we do have pci_lock that protects this, whether the
write/read occurs in the driver or in
pci_generic_config_read32/write32.

Bjorn

On Mon, 2020-11-30 at 11:30 -0600, Bjorn Helgaas wrote:
> [+cc Lukas, pciehp power control question]

> 

> On Mon, Nov 23, 2020 at 02:45:13PM +0800, Jianjun Wang wrote:

> > On Thu, 2020-11-19 at 14:28 -0600, Bjorn Helgaas wrote:

> > > "Add new generation" really contains no information.  And "mediatek"

> > > is already used for the pcie-mediatek.c driver, so we should have a

> > > new tag for this new driver.  Include useful information in the

> > > subject, e.g.,

> > > 

> > >   PCI: mediatek-gen3: Add MediaTek Gen3 driver for MT8192

> 

> > > > +static int mtk_pcie_setup(struct mtk_pcie_port *port)

> > > > +{

> > > > +	struct device *dev = port->dev;

> > > > +	struct platform_device *pdev = to_platform_device(dev);

> > > > +	struct resource *regs;

> > > > +	int err;

> > > > +

> > > > +	regs = platform_get_resource_byname(pdev, IORESOURCE_MEM, "pcie-mac");

> > > > +	port->base = devm_ioremap_resource(dev, regs);

> > > > +	if (IS_ERR(port->base)) {

> > > > +		dev_notice(dev, "failed to map register base\n");

> > > > +		return PTR_ERR(port->base);

> > > > +	}

> > > > +

> > > > +	port->reg_base = regs->start;

> > > > +

> > > > +	/* Don't touch the hardware registers before power up */

> > > > +	err = mtk_pcie_power_up(port);

> > > > +	if (err)

> > > > +		return err;

> > > > +

> > > > +	/* Try link up */

> > > > +	err = mtk_pcie_startup_port(port);

> > > > +	if (err) {

> > > > +		dev_notice(dev, "PCIe link down\n");

> > > > +		goto err_setup;

> > > 

> > > Generally it should not be a fatal error if the link is not up at

> > > probe-time.  You may be able to hot-add a device, or the device may

> > > have some external power control that will power it up later.

> > 

> > This is for the power saving requirement. If there is no device

> > connected with the PCIe slot, the PCIe MAC and PHY should be powered

> > off.

> > 

> > Is there any standard flow to support power down the hardware at

> > probe-time if no device is connected and power it up when hot-add a

> > device?

> 

> That's a good question.  I assume this looks like a standard PCIe

> hot-add event?

> 

> When you hot-add a device, does the Root Port generate a Presence

> Detect Changed interrupt?  The pciehp driver should field that

> interrupt and turn on power to the slot via the Power Controller

> Control bit in the Slot Control register.

> 

> Does your hardware require something more than that to control the MAC

> and PHY power?

> 

> Bjorn


The hardware support to generate a Presence Detect Changed interrupt
when hot-add a device.

But it seems that we should keep the PHY's power and clocks to ensure
the data link layer state change can be detected, and keep the MAC layer
active for routing the interrupt event to pciehp driver handler.

For the power saving requirement, the modules that is not used when
probe-time must be powered off, so I think we may not support hot-plug
in this case.

Thanks

On Mon, Nov 30, 2020 at 11:30:05AM -0600, Bjorn Helgaas wrote:
> On Mon, Nov 23, 2020 at 02:45:13PM +0800, Jianjun Wang wrote:

> > On Thu, 2020-11-19 at 14:28 -0600, Bjorn Helgaas wrote:

> > > > +static int mtk_pcie_setup(struct mtk_pcie_port *port)

> > > > +{

[...]
> > > > +	/* Try link up */

> > > > +	err = mtk_pcie_startup_port(port);

> > > > +	if (err) {

> > > > +		dev_notice(dev, "PCIe link down\n");

> > > > +		goto err_setup;

> > > 

> > > Generally it should not be a fatal error if the link is not up at

> > > probe-time.  You may be able to hot-add a device, or the device may

> > > have some external power control that will power it up later.

> > 

> > This is for the power saving requirement. If there is no device

> > connected with the PCIe slot, the PCIe MAC and PHY should be powered

> > off.

> > 

> > Is there any standard flow to support power down the hardware at

> > probe-time if no device is connected and power it up when hot-add a

> > device?

> 

> That's a good question.  I assume this looks like a standard PCIe

> hot-add event?

> 

> When you hot-add a device, does the Root Port generate a Presence

> Detect Changed interrupt?  The pciehp driver should field that

> interrupt and turn on power to the slot via the Power Controller

> Control bit in the Slot Control register.

> 

> Does your hardware require something more than that to control the MAC

> and PHY power?


Power saving of unused PCIe ports is generally achieved through the
runtime PM framework.  When a PCIe port runtime suspends, the PCIe
core will transition it to D3hot.  On top of that, the platform
may be able to transition the port to D3cold.  Currently only the
ACPI platform supports that.  Conceivably, devicetree-based systems
may want to disable certain clocks or regulators when a PCIe port
runtime suspends.  I think we do not support that yet but it could
be added to drivers/pci/pcie/portdrv*.

A hotplug port is expected to signal PDC and DLLSC interrupts even
when in D3hot.  At least that's our experience with Thunderbolt.
To support hotplug interrupts in D3cold, some external mechanism
(such as a PME) is necessary to wake up the port on hotplug.
This is also supported with recent Thunderbolt systems.

Because we've seen various incompatibilities when runtime suspending
PCIe ports, certain conditions must be satisfied for runtime PM
to be enabled.  They're encoded in pci_bridge_d3_possible().
Generally, hotplug ports only runtime suspend if they belong to
a Thunderbolt controller or if the ACPI platform explicitly allows
runtime PM (through presence of a _PR3 method or a device property).
Non-hotplug ports runtime suspend if the BIOS is newer than 2015
(as specified by DMI).

Obviously, this policy is very x86-focussed because both Thunderbolt
and DMI are only really a thing on x86.  That's about to change though
because Apple's new arm64-based Macs have Thunderbolt integrated into
the SoC and arm64 SoCs are making inroads in the datacenter, which is
an important use case for PCIe hotplug (hot-swappable NVMe drives).
So we may have to amend pci_bridge_d3_possible() to whitelist
PCIe ports for runtime PM on specific arches or systems.

Thanks,

Lukas

On Fri, Dec 04, 2020 at 08:39:09AM +0100, Lukas Wunner wrote:
> On Mon, Nov 30, 2020 at 11:30:05AM -0600, Bjorn Helgaas wrote:

> > On Mon, Nov 23, 2020 at 02:45:13PM +0800, Jianjun Wang wrote:

> > > On Thu, 2020-11-19 at 14:28 -0600, Bjorn Helgaas wrote:

> > > > > +static int mtk_pcie_setup(struct mtk_pcie_port *port)

> > > > > +{

> [...]

> > > > > +	/* Try link up */

> > > > > +	err = mtk_pcie_startup_port(port);

> > > > > +	if (err) {

> > > > > +		dev_notice(dev, "PCIe link down\n");

> > > > > +		goto err_setup;

> > > > 

> > > > Generally it should not be a fatal error if the link is not up at

> > > > probe-time.  You may be able to hot-add a device, or the device may

> > > > have some external power control that will power it up later.

> > > 

> > > This is for the power saving requirement. If there is no device

> > > connected with the PCIe slot, the PCIe MAC and PHY should be powered

> > > off.

> > > 

> > > Is there any standard flow to support power down the hardware at

> > > probe-time if no device is connected and power it up when hot-add a

> > > device?

> > 

> > That's a good question.  I assume this looks like a standard PCIe

> > hot-add event?

> > 

> > When you hot-add a device, does the Root Port generate a Presence

> > Detect Changed interrupt?  The pciehp driver should field that

> > interrupt and turn on power to the slot via the Power Controller

> > Control bit in the Slot Control register.

> > 

> > Does your hardware require something more than that to control the MAC

> > and PHY power?

> 

> Power saving of unused PCIe ports is generally achieved through the

> runtime PM framework.  When a PCIe port runtime suspends, the PCIe

> core will transition it to D3hot.  On top of that, the platform

> may be able to transition the port to D3cold.  Currently only the

> ACPI platform supports that.  Conceivably, devicetree-based systems

> may want to disable certain clocks or regulators when a PCIe port

> runtime suspends.  I think we do not support that yet but it could

> be added to drivers/pci/pcie/portdrv*.

> 

> A hotplug port is expected to signal PDC and DLLSC interrupts even

> when in D3hot.  At least that's our experience with Thunderbolt.

> To support hotplug interrupts in D3cold, some external mechanism

> (such as a PME) is necessary to wake up the port on hotplug.

> This is also supported with recent Thunderbolt systems.

> 

> Because we've seen various incompatibilities when runtime suspending

> PCIe ports, certain conditions must be satisfied for runtime PM

> to be enabled.  They're encoded in pci_bridge_d3_possible().

> Generally, hotplug ports only runtime suspend if they belong to

> a Thunderbolt controller or if the ACPI platform explicitly allows

> runtime PM (through presence of a _PR3 method or a device property).

> Non-hotplug ports runtime suspend if the BIOS is newer than 2015

> (as specified by DMI).

> 

> Obviously, this policy is very x86-focussed because both Thunderbolt

> and DMI are only really a thing on x86.  That's about to change though

> because Apple's new arm64-based Macs have Thunderbolt integrated into

> the SoC and arm64 SoCs are making inroads in the datacenter, which is

> an important use case for PCIe hotplug (hot-swappable NVMe drives).

> So we may have to amend pci_bridge_d3_possible() to whitelist

> PCIe ports for runtime PM on specific arches or systems.


Thanks for all this very useful information!

My interpretation for the mediatek situation:

  - I assume this patch leaves or puts the Root Port in D3cold if no
    downstream devices are present.

  - I don't see any support for PME or similar mechanisms to signal a
    hot-add while the RP is in D3cold.

  - So I assume you don't support hot-add if the slot was empty at
    boot and that's acceptable for your platform.

On Fri, 2020-12-04 at 12:30 -0600, Bjorn Helgaas wrote:
> On Fri, Dec 04, 2020 at 08:39:09AM +0100, Lukas Wunner wrote:

> > On Mon, Nov 30, 2020 at 11:30:05AM -0600, Bjorn Helgaas wrote:

> > > On Mon, Nov 23, 2020 at 02:45:13PM +0800, Jianjun Wang wrote:

> > > > On Thu, 2020-11-19 at 14:28 -0600, Bjorn Helgaas wrote:

> > > > > > +static int mtk_pcie_setup(struct mtk_pcie_port *port)

> > > > > > +{

> > [...]

> > > > > > +	/* Try link up */

> > > > > > +	err = mtk_pcie_startup_port(port);

> > > > > > +	if (err) {

> > > > > > +		dev_notice(dev, "PCIe link down\n");

> > > > > > +		goto err_setup;

> > > > > 

> > > > > Generally it should not be a fatal error if the link is not up at

> > > > > probe-time.  You may be able to hot-add a device, or the device may

> > > > > have some external power control that will power it up later.

> > > > 

> > > > This is for the power saving requirement. If there is no device

> > > > connected with the PCIe slot, the PCIe MAC and PHY should be powered

> > > > off.

> > > > 

> > > > Is there any standard flow to support power down the hardware at

> > > > probe-time if no device is connected and power it up when hot-add a

> > > > device?

> > > 

> > > That's a good question.  I assume this looks like a standard PCIe

> > > hot-add event?

> > > 

> > > When you hot-add a device, does the Root Port generate a Presence

> > > Detect Changed interrupt?  The pciehp driver should field that

> > > interrupt and turn on power to the slot via the Power Controller

> > > Control bit in the Slot Control register.

> > > 

> > > Does your hardware require something more than that to control the MAC

> > > and PHY power?

> > 

> > Power saving of unused PCIe ports is generally achieved through the

> > runtime PM framework.  When a PCIe port runtime suspends, the PCIe

> > core will transition it to D3hot.  On top of that, the platform

> > may be able to transition the port to D3cold.  Currently only the

> > ACPI platform supports that.  Conceivably, devicetree-based systems

> > may want to disable certain clocks or regulators when a PCIe port

> > runtime suspends.  I think we do not support that yet but it could

> > be added to drivers/pci/pcie/portdrv*.

> > 

> > A hotplug port is expected to signal PDC and DLLSC interrupts even

> > when in D3hot.  At least that's our experience with Thunderbolt.

> > To support hotplug interrupts in D3cold, some external mechanism

> > (such as a PME) is necessary to wake up the port on hotplug.

> > This is also supported with recent Thunderbolt systems.

> > 

> > Because we've seen various incompatibilities when runtime suspending

> > PCIe ports, certain conditions must be satisfied for runtime PM

> > to be enabled.  They're encoded in pci_bridge_d3_possible().

> > Generally, hotplug ports only runtime suspend if they belong to

> > a Thunderbolt controller or if the ACPI platform explicitly allows

> > runtime PM (through presence of a _PR3 method or a device property).

> > Non-hotplug ports runtime suspend if the BIOS is newer than 2015

> > (as specified by DMI).

> > 

> > Obviously, this policy is very x86-focussed because both Thunderbolt

> > and DMI are only really a thing on x86.  That's about to change though

> > because Apple's new arm64-based Macs have Thunderbolt integrated into

> > the SoC and arm64 SoCs are making inroads in the datacenter, which is

> > an important use case for PCIe hotplug (hot-swappable NVMe drives).

> > So we may have to amend pci_bridge_d3_possible() to whitelist

> > PCIe ports for runtime PM on specific arches or systems.

> 

> Thanks for all this very useful information!

> 

> My interpretation for the mediatek situation:

> 

>   - I assume this patch leaves or puts the Root Port in D3cold if no

>     downstream devices are present.

> 

>   - I don't see any support for PME or similar mechanisms to signal a

>     hot-add while the RP is in D3cold.

> 

>   - So I assume you don't support hot-add if the slot was empty at

>     boot and that's acceptable for your platform.


Yes, the hardware of Root Port will be totally powered off by gating its
MTCMOS and clocks if the slot is empty at boot time. Because we are
focus on the scenario of power saving, it's acceptable if we don't
support hot-add.

Thanks.