Message-ID: <1501835976.24341.21.camel@mtksdaap41>
Date:   Fri, 4 Aug 2017 16:39:36 +0800
From:   Honghui Zhang <honghui.zhang@...iatek.com>
To:     Bjorn Helgaas <helgaas@...nel.org>
CC:     <bhelgaas@...gle.com>, <robh@...enl.org>, <robh+dt@...nel.org>,
        <matthias.bgg@...il.com>, <linux-arm-kernel@...ts.infradead.org>,
        <linux-mediatek@...ts.infradead.org>, <linux-pci@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <devicetree@...r.kernel.org>,
        <yingjoe.chen@...iatek.com>, <eddie.huang@...iatek.com>,
        <ryder.lee@...iatek.com>, <hongkun.cao@...iatek.com>,
        <youlin.pei@...iatek.com>, <yong.wu@...iatek.com>,
        <yt.shen@...iatek.com>, <sean.wang@...iatek.com>,
        <xinping.qian@...iatek.com>
Subject: Re: [PATCH v2 4/5] PCI: mediatek: Add new generation controller
 support

Hi, Bjorn,
	Thanks very much for your reviews.
The mis-spells will be fixed in next version.


On Thu, 2017-08-03 at 17:42 -0500, Bjorn Helgaas wrote:


......
> > +}
> > +
> > +static struct mtk_pcie_port *mtk_pcie_find_port(struct mtk_pcie *pcie,
> > +						struct pci_bus *bus, int devfn)
> > +{
> > +	struct pci_dev *dev;
> > +	struct pci_bus *pbus;
> > +	struct mtk_pcie_port *port, *tmp;
> > +
> > +	list_for_each_entry_safe(port, tmp, &pcie->ports, list) {
> > +		if (bus->number == 0 && port->index == PCI_SLOT(devfn)) {
> > +			return port;
> > +		} else if (bus->number != 0) {
> > +			pbus = bus;
> > +			do {
> > +				dev = pbus->self;
> > +				if (port->index == PCI_SLOT(dev->devfn))
> > +					return port;
> > +				pbus = dev->bus;
> > +			} while (dev->bus->number != 0);
> > +		}
> > +	}
> > +
> > +	return NULL;
> 
> You should be able to use sysdata to avoid searching the list.
> See drivers/pci/host/pci-aardvark.c, for example.
> 

I could put the mtk_pcie * in sysdata, but still need to searching the
list to get the mtk_pcie_port *, how about:

	list_for_each_entry_safe(port, tmp, &pcie->ports, list) {
		if (port->index == PCI_SLOT(devfn))
			return port;
	}

> > +}
> > +
> > +static int mtk_pcie_config_read(struct pci_bus *bus, unsigned int devfn,
> > +				int where, int size, u32 *val)
> > +{
> > +	struct mtk_pcie_port *port;
> > +	struct pci_host_bridge *host = pci_find_host_bridge(bus);
> > +	struct mtk_pcie *pcie = pci_host_bridge_priv(host);
> 
> Sysdata should make this very simple; see advk_pcie_rd_conf().

thanks.

> 
> > +	u32 bn = bus->number;
> > +	int ret;
> > +
> > +	port = mtk_pcie_find_port(pcie, bus, devfn);
> > +	if (!port) {
> > +		*val = ~0;
> > +		return PCIBIOS_DEVICE_NOT_FOUND;
> > +	}
> > +
> > +	ret = mtk_pcie_hw_rd_cfg(port, bn, devfn, where, size, val);
> > +	if (ret)
> > +		*val = ~0;
> > +
> > +	return ret;
> > +}
> > +
> > +static int mtk_pcie_config_write(struct pci_bus *bus, unsigned int devfn,
> > +				 int where, int size, u32 val)
> > +{
> > +	u32 bn = bus->number;
> > +	struct pci_host_bridge *host = pci_find_host_bridge(bus);
> > +	struct mtk_pcie *pcie = pci_host_bridge_priv(host);
> > +	struct mtk_pcie_port *port;
> > +
> > +	port = mtk_pcie_find_port(pcie, bus, devfn);
> > +	if (!port)
> > +		return PCIBIOS_DEVICE_NOT_FOUND;
> > +
> > +	return mtk_pcie_hw_wr_cfg(port, bn, devfn, where, size, val);
> > +}
> > +
> > +static struct pci_ops mtk_pcie_ops_v2 = {
> > +	.read  = mtk_pcie_config_read,
> > +	.write = mtk_pcie_config_write,
> > +};
> > +
> > +static int mtk_pcie_startup_ports_v2(struct mtk_pcie_port *port)
> > +{
> > +	struct mtk_pcie *pcie = port->pcie;
> > +	struct resource *mem = &pcie->mem;
> > +	u32 val;
> > +	size_t size;
> > +	int err;
> > +
> > +	/* mt7622 platforms need to enable LTSSM and ASPM from PCIe subsys */
> > +	if (pcie->base) {
> > +		val = readl(pcie->base + PCIE_SYS_CFG_V2);
> > +		val |= PCIE_CSR_LTSSM_EN(port->index) |
> > +		       PCIE_CSR_ASPM_L1_EN(port->index);
> > +		writel(val, pcie->base + PCIE_SYS_CFG_V2);
> > +	}
> > +
> > +	/* Assert all reset signals */
> > +	writel(0, port->base + PCIE_RST_CTRL);
> > +
> > +	/*
> > +	 * Enable rc internal reset.
> > +	 * The reset will work when the link is from link up to link down.
> 
> ?  That sentence doesn't parse for me.

What about:

	/*
	 * Enable PCIe link down reset, if link status changed from link up to
	 * link down, this will reset MAC control registers and configuration
	 * space.
	 */

> 
> > +	 */
> > +	writel(PCI_LINKDOWN_RST_EN, port->base + PCIE_RST_CTRL);
> > +
> > +	/* De-assert phy, pe, pipe, mac and configuration reset	*/
> > +	val = readl(port->base + PCIE_RST_CTRL);
> > +	val |= PCIE_PHY_RSTB | PCIE_PERSTB | PCIE_PIPE_SRSTB |
> > +	       PCIE_MAC_SRSTB | PCIE_CRSTB;
> > +	writel(val, port->base + PCIE_RST_CTRL);
> > +
> > +	/* PCIe v2.0 need at least 100ms delay to train from Gen1 to Gen2 */
> > +	err = readl_poll_timeout(port->base + PCIE_LINK_STATUS_V2, val,
> > +				 !!(val & PCIE_PORT_LINKUP_V2), 20,
> > +				 100 * USEC_PER_MSEC);
> > +	if (err)
> > +		return -ETIMEDOUT;
> > +
> > +	/* Set INTx mask */
> > +	val = readl(port->base + PCIE_INT_MASK);
> > +	val &= ~INTX_MASK;
> > +	writel(val, port->base + PCIE_INT_MASK);
> > +
> > +	/* Set AHB to PCIe translation windows */
> > +	size = mem->end - mem->start;
> > +	val = AHB2PCIE_BASEL(mem->start) | AHB2PCIE_SIZE(fls(size));
> > +	writel(val, port->base + PCIE_AHB_TRANS_BASE0_L);
> > +
> > +	val = AHB2PCIE_BASEH(mem->start);
> > +	writel(val, port->base + PCIE_AHB_TRANS_BASE0_H);
> > +
> > +	/* Set PCIe to axi translation memory space.*/
> 
> s/axi/AXI/
> 
> > +	val = fls(0xffffffff) | WIN_ENABLE;
> > +	writel(val, port->base + PCIE_AXI_WINDOW0);
> > +
> > +	return 0;
> > +}
> > +
> > +static int mtk_pcie_intx_map(struct irq_domain *domain, unsigned int irq,
> > +			     irq_hw_number_t hwirq)
> > +{
> > +	irq_set_chip_and_handler(irq, &dummy_irq_chip, handle_simple_irq);
> > +	irq_set_chip_data(irq, domain->host_data);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct irq_domain_ops intx_domain_ops = {
> > +	.map = mtk_pcie_intx_map,
> > +};
> > +
> > +static int mtk_pcie_init_irq_domain(struct mtk_pcie_port *port,
> > +				    struct device_node *node)
> > +{
> > +	struct device *dev = port->pcie->dev;
> > +	struct device_node *pcie_intc_node;
> > +
> > +	/* Setup INTx */
> > +	pcie_intc_node = of_get_next_child(node, NULL);
> > +	if (!pcie_intc_node) {
> > +		dev_err(dev, "No PCIe Intc node found\n");
> > +		return PTR_ERR(pcie_intc_node);
> > +	}
> > +
> > +	port->irq_domain = irq_domain_add_linear(pcie_intc_node, INTX_NUM,
> > +						 &intx_domain_ops, port);
> 
> I think there's an issue here with a 4-element IRQ domain and the
> hwirq numbers 1-4 from the of_irq_parse_and_map_pci() path, so INTD
> may not work correctly.
> 
> See
> http://lkml.kernel.org/r/20170801212931.GA26498@bhelgaas-glaptop.roam.corp.google.com
> and related discussion.
> 

Sorry, I did not get this,
I do some test with an intel E350T4 PCIe NICs, it's a x1 lane
multi-function device.
What I got from the log is below:
->of_irq_parse_and_map_pci
	->of_irq_parse_pci
		->irq_create_of_mapping
			->irq_create_fwspec_mapping
				->irq_domain_translate
				which will go through
				d->ops->translate #the hwirq really start from 0

And I tested every NIC port of the Intel E350T4 with tftp transfer data,
seems all are OK with this code.

What I got from the proc is as below:
cat /proc/interrupts
           CPU0       CPU1       CPU2       
  1:          0          0          0     GICv2  25 Level     vgic
  3:       5042        224        206     GICv2  30 Level     arch_timer
  4:          0          0          0     GICv2  27 Level     kvm guest
timer
  6:        201          0          0  MT_SYSIRQ  91 Level     ttyS0
  7:         57          0          0  MT_SYSIRQ 115 Level     mtk-pcie
  8:          0          0          0  MT_SYSIRQ 117 Level     mtk-pcie
  9:          9          0          0     dummy   0 Edge      eth0
 10:         40          0          0     dummy   1 Edge      eth1
 11:          5          0          0     dummy   2 Edge      eth2
 12:          3          0          0     dummy   3 Edge      eth3
IPI0:       314        507       1164       Rescheduling interrupts

or did I missed something?


> > +	if (!port->irq_domain) {
> > +		dev_err(dev, "Failed to get INTx IRQ domain\n");
> > +		return PTR_ERR(port->irq_domain);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static irqreturn_t mtk_pcie_intr_handler(int irq, void *data)
> > +{
> > +	struct mtk_pcie_port *port = (struct mtk_pcie_port *)data;
> > +	struct device *dev = port->pcie->dev;
> > +	unsigned long status;
> > +	u32 virq;
> > +	u32 bit = INTX_SHIFT;
> > +
> > +	while ((status = readl(port->base + PCIE_INT_STATUS)) & INTX_MASK) {
> > +		for_each_set_bit_from(bit, &status, INTX_NUM + INTX_SHIFT) {
> > +			/* Clear the INTx */
> > +			writel(1 << bit, port->base + PCIE_INT_STATUS);
> > +			virq = irq_find_mapping(port->irq_domain,
> > +						bit - INTX_SHIFT);
> > +			if (virq)
> > +				generic_handle_irq(virq);
> > +			else
> > +				dev_err(dev, "unexpected IRQ, INT%d\n",
> > +					bit - INTX_SHIFT);
> 
> PCI INTx are conventionally INTA, INTB, INTC, INTD (not INT1, INT2,
> etc).
> 
> > +		}
> > +	}
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +static int mtk_pcie_setup_irq(struct mtk_pcie_port *port,
> > +			      struct device_node *node)
> > +{
> > +	struct mtk_pcie *pcie = port->pcie;
> > +	struct device *dev = pcie->dev;
> > +	struct platform_device *pdev = to_platform_device(dev);
> > +	int err, irq;
> > +
> > +	irq = platform_get_irq(pdev, port->index);
> > +	err = devm_request_irq(dev, irq, mtk_pcie_intr_handler,
> > +			       IRQF_SHARED, "mtk-pcie", port);
> > +	if (err) {
> > +		dev_err(dev, "unable to request irq %d\n", irq);
> 
> s/irq/IRQ/
> 
> > +		return err;
> > +	}
> > +
> > +	err = mtk_pcie_init_irq_domain(port, node);
> > +	if (err) {
> > +		dev_err(dev, "failed to init pcie lagecy irq domain\n");
> 
> s/lagecy/legacy/
> s/irq/IRQ/
> s/pcie/PCIe/
> 
> > +		return -ENODEV;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >  static void __iomem *mtk_pcie_map_bus(struct pci_bus *bus,
> >  				      unsigned int devfn, int where)
> >  {
> > @@ -249,13 +625,49 @@ static void mtk_pcie_enable_ports(struct mtk_pcie_port *port)
> >  
> >  	err = clk_prepare_enable(port->sys_ck);
> >  	if (err) {
> > -		dev_err(dev, "failed to enable port%d clock\n", port->index);
> > +		dev_err(dev, "failed to enable sys_ck%d\n", port->index);
> >  		goto err_sys_clk;
> >  	}
> >  
> > +	err = clk_prepare_enable(port->ahb_ck);
> > +	if (err) {
> > +		dev_err(dev, "failed to enable ahb_ck%d\n", port->index);
> > +		goto err_ahb_clk;
> > +	}
> > +
> > +	err = clk_prepare_enable(port->aux_ck);
> > +	if (err) {
> > +		dev_err(dev, "failed to enable aux_ck%d\n", port->index);
> > +		goto err_aux_clk;
> > +	}
> > +
> > +	err = clk_prepare_enable(port->axi_ck);
> > +	if (err) {
> > +		dev_err(dev, "failed to enable axi_ck%d\n", port->index);
> > +		goto err_axi_clk;
> > +	}
> > +
> > +	err = clk_prepare_enable(port->obff_ck);
> > +	if (err) {
> > +		dev_err(dev, "failed to enable obff_ck%d\n", port->index);
> > +		goto err_obff_clk;
> > +	}
> > +
> > +	err = clk_prepare_enable(port->pipe_ck);
> > +	if (err) {
> > +		dev_err(dev, "failed to enable pipe_ck%d\n", port->index);
> > +		goto err_pipe_clk;
> > +	}
> > +
> >  	reset_control_assert(port->reset);
> >  	reset_control_deassert(port->reset);
> >  
> > +	err = phy_init(port->phy);
> > +	if (err) {
> > +		dev_err(dev, "failed to initialize port%d phy\n", port->index);
> > +		goto err_phy_init;
> > +	}
> > +
> >  	err = phy_power_on(port->phy);
> >  	if (err) {
> >  		dev_err(dev, "failed to power on port%d phy\n", port->index);
> > @@ -269,6 +681,18 @@ static void mtk_pcie_enable_ports(struct mtk_pcie_port *port)
> >  
> >  	phy_power_off(port->phy);
> >  err_phy_on:
> > +	phy_exit(port->phy);
> > +err_phy_init:
> > +	clk_disable_unprepare(port->pipe_ck);
> > +err_pipe_clk:
> > +	clk_disable_unprepare(port->obff_ck);
> > +err_obff_clk:
> > +	clk_disable_unprepare(port->axi_ck);
> > +err_axi_clk:
> > +	clk_disable_unprepare(port->aux_ck);
> > +err_aux_clk:
> > +	clk_disable_unprepare(port->ahb_ck);
> > +err_ahb_clk:
> >  	clk_disable_unprepare(port->sys_ck);
> >  err_sys_clk:
> >  	mtk_pcie_port_free(port);
> > @@ -306,10 +730,56 @@ static int mtk_pcie_parse_ports(struct mtk_pcie *pcie,
> >  	snprintf(name, sizeof(name), "sys_ck%d", index);
> >  	port->sys_ck = devm_clk_get(dev, name);
> >  	if (IS_ERR(port->sys_ck)) {
> > -		dev_err(dev, "failed to get port%d clock\n", index);
> > +		dev_err(dev, "failed to get sys_ck%d\n", index);
> >  		return PTR_ERR(port->sys_ck);
> >  	}
> >  
> > +	/* sys_ck might be divided into the following parts in some chips */
> > +	snprintf(name, sizeof(name), "ahb_ck%d", index);
> > +	port->ahb_ck = devm_clk_get(dev, name);
> > +	if (IS_ERR(port->ahb_ck)) {
> > +		if (PTR_ERR(port->ahb_ck) == -EPROBE_DEFER)
> > +			return -EPROBE_DEFER;
> > +
> > +		port->ahb_ck = NULL;
> > +	}
> > +
> > +	snprintf(name, sizeof(name), "axi_ck%d", index);
> > +	port->axi_ck = devm_clk_get(dev, name);
> > +	if (IS_ERR(port->axi_ck)) {
> > +		if (PTR_ERR(port->axi_ck) == -EPROBE_DEFER)
> > +			return -EPROBE_DEFER;
> > +
> > +		port->axi_ck = NULL;
> > +	}
> > +
> > +	snprintf(name, sizeof(name), "aux_ck%d", index);
> > +	port->aux_ck = devm_clk_get(dev, name);
> > +	if (IS_ERR(port->aux_ck)) {
> > +		if (PTR_ERR(port->aux_ck) == -EPROBE_DEFER)
> > +			return -EPROBE_DEFER;
> > +
> > +		port->aux_ck = NULL;
> > +	}
> > +
> > +	snprintf(name, sizeof(name), "obff_ck%d", index);
> > +	port->obff_ck = devm_clk_get(dev, name);
> > +	if (IS_ERR(port->obff_ck)) {
> > +		if (PTR_ERR(port->obff_ck) == -EPROBE_DEFER)
> > +			return -EPROBE_DEFER;
> > +
> > +		port->obff_ck = NULL;
> > +	}
> > +
> > +	snprintf(name, sizeof(name), "pipe_ck%d", index);
> > +	port->pipe_ck = devm_clk_get(dev, name);
> > +	if (IS_ERR(port->pipe_ck)) {
> > +		if (PTR_ERR(port->pipe_ck) == -EPROBE_DEFER)
> > +			return -EPROBE_DEFER;
> > +
> > +		port->pipe_ck = NULL;
> > +	}
> > +
> >  	snprintf(name, sizeof(name), "pcie-rst%d", index);
> >  	port->reset = devm_reset_control_get_optional(dev, name);
> >  	if (PTR_ERR(port->reset) == -EPROBE_DEFER)
> > @@ -324,6 +794,12 @@ static int mtk_pcie_parse_ports(struct mtk_pcie *pcie,
> >  	port->index = index;
> >  	port->pcie = pcie;
> >  
> > +	if (pcie->soc->setup_irq) {
> > +		err = pcie->soc->setup_irq(port, node);
> > +		if (err)
> > +			return err;
> > +	}
> > +
> >  	INIT_LIST_HEAD(&port->list);
> >  	list_add_tail(&port->list, &pcie->ports);
> >  
> > @@ -553,9 +1029,17 @@ static struct mtk_pcie_soc mtk_pcie_soc_v1 = {
> >  	.startup = mtk_pcie_startup_ports,
> >  };
> >  
> > +static struct mtk_pcie_soc mtk_pcie_soc_v2 = {
> > +	.ops = &mtk_pcie_ops_v2,
> > +	.startup = mtk_pcie_startup_ports_v2,
> > +	.setup_irq = mtk_pcie_setup_irq,
> > +};
> > +
> >  static const struct of_device_id mtk_pcie_ids[] = {
> >  	{ .compatible = "mediatek,mt2701-pcie", .data = &mtk_pcie_soc_v1 },
> >  	{ .compatible = "mediatek,mt7623-pcie", .data = &mtk_pcie_soc_v1 },
> > +	{ .compatible = "mediatek,mt2712-pcie", .data = &mtk_pcie_soc_v2 },
> > +	{ .compatible = "mediatek,mt7622-pcie", .data = &mtk_pcie_soc_v2 },
> >  	{},
> >  };
> >  
> > -- 
> > 2.6.4
> > 

Powered by blists - more mailing lists