Hello Bjorn, > -----Original Message----- > From: Bjorn Helgaas [mailto:bhelgaas@xxxxxxxxxx] > Sent: Friday, April 04, 2014 10:13 PM > To: Mohit KUMAR DCG > Cc: arnd@xxxxxxxx; Pratyush ANAND; Jingoo Han; Viresh Kumar; spear- > devel; linux-pci@xxxxxxxxxxxxxxx > Subject: Re: [PATCH V7 08/10] pcie: SPEAr13xx: Add designware wrapper > support > > On Fri, Feb 28, 2014 at 05:25:01PM +0530, Mohit Kumar wrote: > > From: Pratyush Anand <pratyush.anand@xxxxxx> > > > > SPEAr1310 and SPEAr1340 SOC uses designware PCIe controller. Add > > SPEAr13xx PCIe driver based on designware controller driver. > > > > SPEAr1310 has 3 PCIe ports and SPEAr1340 has 1, which are multiplexed > > with ahci/sata pins. By default evaluation board of both controller > > works for ahci mode. > > To use these patches on SPEAr1340/1310 evaluation board, do the > > necessary modifications on board and enable (okay) pcie and miphy from > > respective evb dtsi file. > > > > Signed-off-by: Pratyush Anand <pratyush.anand@xxxxxx> > > Signed-off-by: Mohit Kumar <mohit.kumar@xxxxxx> > > Cc: Jingoo Han <jg1.han@xxxxxxxxxxx> > > Cc: Arnd Bergmann <arnd@xxxxxxxx> > > Cc: Viresh Kumar <viresh.linux@xxxxxxxxx> > > Cc: spear-devel@xxxxxxxxxxx > > Cc: linux-pci@xxxxxxxxxxxxxxx > > --- > > arch/arm/boot/dts/spear1310.dtsi | 6 + > > arch/arm/boot/dts/spear1340.dtsi | 2 + > > arch/arm/boot/dts/spear13xx.dtsi | 4 +- > > arch/arm/mach-spear/Kconfig | 1 + > > drivers/pci/host/Kconfig | 8 + > > drivers/pci/host/Makefile | 1 + > > drivers/pci/host/pcie-spear13xx.c | 414 > > +++++++++++++++++++++++++++++++++++++ > > This doesn't apply cleanly to my tree (currently at 4a4389abdd98), apparently > because I don't have some of the ARM DTS stuff. If you want to split this into > a PCI-specific part and an arch/arm part, I can apply the PCI part. Or you can > apply the whole thing via whatever ARM tree makes sense. > Arnd, Pls comment if it can be taken care by you if I resend after rebasing it to your tree? > I do have a couple minor comments below. With those addressed, here's my > ACK so you can merge it via another tree if that makes the most sense: > - Thanks for your review and comments. I will incorporate these in my resend patch. > Acked-by: Bjorn Helgaas <bhelgaas@xxxxxxxxxx> > Thanks Mohit > > diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig index > > 47d46c6..8697dd1 100644 > > --- a/drivers/pci/host/Kconfig > > +++ b/drivers/pci/host/Kconfig > > @@ -33,4 +33,12 @@ config PCI_RCAR_GEN2 > > There are 3 internal PCI controllers available with a single > > built-in EHCI/OHCI host controller present on each one. > > > > +config PCIE_SPEAR13XX > > + tristate "STMicroelectronics SPEAr PCIe controller" > > + depends on ARCH_SPEAR13XX > > + select PCIEPORTBUS > > + select PCIE_DW > > + help > > + Say Y here if you want PCIe support on SPEAr13XX SoCs. > > + > > endmenu > > diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile > > index 13fb333..42a491d 100644 > > --- a/drivers/pci/host/Makefile > > +++ b/drivers/pci/host/Makefile > > @@ -4,3 +4,4 @@ obj-$(CONFIG_PCI_IMX6) += pci-imx6.o > > obj-$(CONFIG_PCI_MVEBU) += pci-mvebu.o > > obj-$(CONFIG_PCI_TEGRA) += pci-tegra.o > > obj-$(CONFIG_PCI_RCAR_GEN2) += pci-rcar-gen2.o > > +obj-$(CONFIG_PCIE_SPEAR13XX) += pcie-spear13xx.o > > diff --git a/drivers/pci/host/pcie-spear13xx.c > > b/drivers/pci/host/pcie-spear13xx.c > > new file mode 100644 > > index 0000000..c70d526 > > --- /dev/null > > +++ b/drivers/pci/host/pcie-spear13xx.c > > @@ -0,0 +1,414 @@ > > +/* > > + * PCIe host controller driver for ST Microelectronics SPEAr13xx SoCs > > + * > > + * SPEAr13xx PCIe Glue Layer Source Code > > + * > > + * Copyright (C) 2010-2014 ST Microelectronics > > + * Pratyush Anand <pratyush.anand@xxxxxx> > > + * Mohit Kumar <mohit.kumar@xxxxxx> > > + * > > + * This file is licensed under the terms of the GNU General Public > > + * License version 2. This program is licensed "as is" without any > > + * warranty of any kind, whether express or implied. > > + */ > > + > > +#include <linux/clk.h> > > +#include <linux/delay.h> > > +#include <linux/interrupt.h> > > +#include <linux/kernel.h> > > +#include <linux/module.h> > > +#include <linux/of.h> > > +#include <linux/pci.h> > > +#include <linux/phy/phy.h> > > +#include <linux/platform_device.h> > > +#include <linux/resource.h> > > + > > +#include "pcie-designware.h" > > + > > +struct spear13xx_pcie { > > + void __iomem *app_base; > > + struct phy *phy; > > + struct clk *clk; > > + struct pcie_port pp; > > + bool is_gen1; > > +}; > > + > > +struct pcie_app_reg { > > + u32 app_ctrl_0; /*cr0*/ > > + u32 app_ctrl_1; /*cr1*/ > > + u32 app_status_0; /*cr2*/ > > + u32 app_status_1; /*cr3*/ > > + u32 msg_status; /*cr4*/ > > + u32 msg_payload; /*cr5*/ > > + u32 int_sts; /*cr6*/ > > + u32 int_clr; /*cr7*/ > > + u32 int_mask; /*cr8*/ > > + u32 mst_bmisc; /*cr9*/ > > + u32 phy_ctrl; /*cr10*/ > > + u32 phy_status; /*cr11*/ > > + u32 cxpl_debug_info_0; /*cr12*/ > > + u32 cxpl_debug_info_1; /*cr13*/ > > + u32 ven_msg_ctrl_0; /*cr14*/ > > + u32 ven_msg_ctrl_1; /*cr15*/ > > + u32 ven_msg_data_0; /*cr16*/ > > + u32 ven_msg_data_1; /*cr17*/ > > + u32 ven_msi_0; /*cr18*/ > > + u32 ven_msi_1; /*cr19*/ > > + u32 mst_rmisc; /*cr 20*/ > > Usually people put a space after "/*" and before "*/" (this applies other > places below, too). But at least make the cr20 comment spacing here > consistent with the other ones above. > > > +}; > > + > > +/*CR0 ID*/ > > +#define RX_LANE_FLIP_EN_ID 0 > > +#define TX_LANE_FLIP_EN_ID 1 > > +#define SYS_AUX_PWR_DET_ID 2 > > +#define APP_LTSSM_ENABLE_ID 3 > > +#define SYS_ATTEN_BUTTON_PRESSED_ID 4 > > +#define SYS_MRL_SENSOR_STATE_ID 5 > > +#define SYS_PWR_FAULT_DET_ID 6 > > +#define SYS_MRL_SENSOR_CHGED_ID 7 > > +#define SYS_PRE_DET_CHGED_ID 8 > > +#define SYS_CMD_CPLED_INT_ID 9 > > +#define APP_INIT_RST_0_ID 11 > > +#define APP_REQ_ENTR_L1_ID 12 > > +#define APP_READY_ENTR_L23_ID 13 > > +#define APP_REQ_EXIT_L1_ID 14 > > +#define DEVICE_TYPE_EP (0 << 25) > > +#define DEVICE_TYPE_LEP (1 << 25) > > +#define DEVICE_TYPE_RC (4 << 25) > > +#define SYS_INT_ID 29 > > +#define MISCTRL_EN_ID 30 > > +#define REG_TRANSLATION_ENABLE 31 > > Many of these symbols are defined but never used. Personally, I would just > define the things you use, since they've presumably gotten at least some > testing. Then the list is a clue to the reader about what features the driver > supports, and you can add others when you add more features. I think > they're more likely to get reviewed and tested then. But not a big deal either > way. > > > +/*CR1 ID*/ > > +#define APPS_PM_XMT_TURNOFF_ID 2 > > +#define APPS_PM_XMT_PME_ID 5 > > + > > +/*CR3 ID*/ > > +#define XMLH_LTSSM_STATE_DETECT_QUIET 0x00 > > +#define XMLH_LTSSM_STATE_DETECT_ACT 0x01 > > +#define XMLH_LTSSM_STATE_POLL_ACTIVE 0x02 > > +#define XMLH_LTSSM_STATE_POLL_COMPLIANCE 0x03 > > +#define XMLH_LTSSM_STATE_POLL_CONFIG 0x04 > > +#define XMLH_LTSSM_STATE_PRE_DETECT_QUIET 0x05 > > +#define XMLH_LTSSM_STATE_DETECT_WAIT 0x06 > > +#define XMLH_LTSSM_STATE_CFG_LINKWD_START 0x07 > > +#define XMLH_LTSSM_STATE_CFG_LINKWD_ACEPT 0x08 > > +#define XMLH_LTSSM_STATE_CFG_LANENUM_WAIT 0x09 > > +#define XMLH_LTSSM_STATE_CFG_LANENUM_ACEPT 0x0A > > +#define XMLH_LTSSM_STATE_CFG_COMPLETE 0x0B > > +#define XMLH_LTSSM_STATE_CFG_IDLE 0x0C > > +#define XMLH_LTSSM_STATE_RCVRY_LOCK 0x0D > > +#define XMLH_LTSSM_STATE_RCVRY_SPEED 0x0E > > +#define XMLH_LTSSM_STATE_RCVRY_RCVRCFG 0x0F > > +#define XMLH_LTSSM_STATE_RCVRY_IDLE 0x10 > > +#define XMLH_LTSSM_STATE_L0 0x11 > > +#define XMLH_LTSSM_STATE_L0S 0x12 > > +#define XMLH_LTSSM_STATE_L123_SEND_EIDLE 0x13 > > +#define XMLH_LTSSM_STATE_L1_IDLE 0x14 > > +#define XMLH_LTSSM_STATE_L2_IDLE 0x15 > > +#define XMLH_LTSSM_STATE_L2_WAKE 0x16 > > +#define XMLH_LTSSM_STATE_DISABLED_ENTRY 0x17 > > +#define XMLH_LTSSM_STATE_DISABLED_IDLE 0x18 > > +#define XMLH_LTSSM_STATE_DISABLED 0x19 > > +#define XMLH_LTSSM_STATE_LPBK_ENTRY 0x1A > > +#define XMLH_LTSSM_STATE_LPBK_ACTIVE 0x1B > > +#define XMLH_LTSSM_STATE_LPBK_EXIT 0x1C > > +#define XMLH_LTSSM_STATE_LPBK_EXIT_TIMEOUT 0x1D > > +#define XMLH_LTSSM_STATE_HOT_RESET_ENTRY 0x1E > > +#define XMLH_LTSSM_STATE_HOT_RESET 0x1F > > +#define XMLH_LTSSM_STATE_MASK 0x3F > > +#define XMLH_LINK_UP (1 << 6) > > + > > +/*CR4 ID*/ > > +#define CFG_MSI_EN_ID 18 > > + > > +/*CR6*/ > > +#define INTA_CTRL_INT (1 << 7) > > +#define INTB_CTRL_INT (1 << 8) > > +#define INTC_CTRL_INT (1 << 9) > > +#define INTD_CTRL_INT (1 << 10) > > +#define MSI_CTRL_INT (1 << 26) > > + > > +/*CR19 ID*/ > > +#define VEN_MSI_REQ_ID 11 > > +#define VEN_MSI_FUN_NUM_ID 8 > > +#define VEN_MSI_TC_ID 5 > > +#define VEN_MSI_VECTOR_ID 0 > > +#define VEN_MSI_REQ_EN ((u32)0x1 << VEN_MSI_REQ_ID) > > +#define VEN_MSI_FUN_NUM_MASK ((u32)0x7 << > VEN_MSI_FUN_NUM_ID) > > +#define VEN_MSI_TC_MASK ((u32)0x7 << > VEN_MSI_TC_ID) > > +#define VEN_MSI_VECTOR_MASK ((u32)0x1F << VEN_MSI_VECTOR_ID) > > + > > +#define PCI_CAP_ID_EXP_OFFSET 0x70 > > + > > +#define to_spear13xx_pcie(x) container_of(x, struct > spear13xx_pcie, pp) > > + > > +static int spear13xx_pcie_establish_link(struct pcie_port *pp) { > > + u32 val; > > + int count = 0; > > + struct spear13xx_pcie *spear13xx_pcie = to_spear13xx_pcie(pp); > > + struct pcie_app_reg *app_reg = spear13xx_pcie->app_base; > > + u32 exp_cap_off = PCI_CAP_ID_EXP_OFFSET; > > + > > + if (dw_pcie_link_up(pp)) { > > + dev_err(pp->dev, "Link already up\n"); > > Most of your messages start with lowercase; it'd be nice to be consistent. > > > + return 0; > > + } > > + > > + /* setup root complex */ > > Superfluous comment. > > > + dw_pcie_setup_rc(pp); > > + > > + /* > > + * this controller support only 128 bytes read size, however its > > + * default value in capability register is 512 bytes. So force > > + * it to 128 here. > > + */ > > + dw_pcie_cfg_read(pp->dbi_base, exp_cap_off + PCI_EXP_DEVCTL, > 4, &val); > > + val &= ~PCI_EXP_DEVCTL_READRQ; > > + dw_pcie_cfg_write(pp->dbi_base, exp_cap_off + PCI_EXP_DEVCTL, > 4, > > +val); > > + > > + /* program vid and did for RC */ > > Superfluous comment. > > > + dw_pcie_cfg_write(pp->dbi_base, PCI_VENDOR_ID, 2, 0x104A); > > + dw_pcie_cfg_write(pp->dbi_base, PCI_DEVICE_ID, 2, 0xCD80); > > + > > + /* > > + * if is_gen1 is set then handle it, so that some buggy card > > + * also works > > + */ > > + if (spear13xx_pcie->is_gen1) { > > + dw_pcie_cfg_read(pp->dbi_base, exp_cap_off + > PCI_EXP_LNKCAP, 4, > > + &val); > > + if ((val & PCI_EXP_LNKCAP_SLS) != > PCI_EXP_LNKCAP_SLS_2_5GB) { > > + val &= ~((u32)PCI_EXP_LNKCAP_SLS); > > + val |= PCI_EXP_LNKCAP_SLS_2_5GB; > > + dw_pcie_cfg_write(pp->dbi_base, exp_cap_off + > > + PCI_EXP_LNKCAP, 4, val); > > + } > > + > > + dw_pcie_cfg_read(pp->dbi_base, exp_cap_off + > PCI_EXP_LNKCTL2, 4, > > + &val); > > + if ((val & PCI_EXP_LNKCAP_SLS) != > PCI_EXP_LNKCAP_SLS_2_5GB) { > > + val &= ~((u32)PCI_EXP_LNKCAP_SLS); > > + val |= PCI_EXP_LNKCAP_SLS_2_5GB; > > + dw_pcie_cfg_write(pp->dbi_base, exp_cap_off + > > + PCI_EXP_LNKCTL2, 4, val); > > + } > > + } > > + > > + /* enable ltssm */ > > + writel(DEVICE_TYPE_RC | (1 << MISCTRL_EN_ID) > > + | (1 << APP_LTSSM_ENABLE_ID) > > + | ((u32)1 << REG_TRANSLATION_ENABLE), > > + &app_reg->app_ctrl_0); > > + > > + /* check if the link is up or not */ > > + while (!dw_pcie_link_up(pp)) { > > + mdelay(100); > > + count++; > > + if (count == 10) { > > + dev_err(pp->dev, "Link Fail\n"); > > + return -EINVAL; > > + } > > + } > > + dev_info(pp->dev, "Link up\n"); > > + > > + return 0; > > +} > > + > > +static irqreturn_t spear13xx_pcie_irq_handler(int irq, void *arg) { > > + struct pcie_port *pp = arg; > > + struct spear13xx_pcie *spear13xx_pcie = to_spear13xx_pcie(pp); > > + struct pcie_app_reg *app_reg = spear13xx_pcie->app_base; > > + unsigned int status; > > + > > + status = readl(&app_reg->int_sts); > > + > > + if (status & MSI_CTRL_INT) { > > + if (!IS_ENABLED(CONFIG_PCI_MSI)) > > + BUG(); > > + dw_handle_msi_irq(pp); > > + } > > + > > + writel(status, &app_reg->int_clr); > > + > > + return IRQ_HANDLED; > > +} > > + > > +static void spear13xx_pcie_enable_interrupts(struct pcie_port *pp) { > > + struct spear13xx_pcie *spear13xx_pcie = to_spear13xx_pcie(pp); > > + struct pcie_app_reg *app_reg = spear13xx_pcie->app_base; > > + > > + /* Enable MSI interrupt */ > > + if (IS_ENABLED(CONFIG_PCI_MSI)) { > > + dw_pcie_msi_init(pp); > > + writel(readl(&app_reg->int_mask) | > > + MSI_CTRL_INT, &app_reg->int_mask); > > + } > > + > > + return; > > Superfluous "return". > > > +} > > + > > +static int spear13xx_pcie_link_up(struct pcie_port *pp) { > > + struct spear13xx_pcie *spear13xx_pcie = to_spear13xx_pcie(pp); > > + struct pcie_app_reg *app_reg = spear13xx_pcie->app_base; > > + > > + if (readl(&app_reg->app_status_1) & XMLH_LINK_UP) > > + return 1; > > + > > + return 0; > > +} > > + > > +static void spear13xx_pcie_host_init(struct pcie_port *pp) { > > + spear13xx_pcie_establish_link(pp); > > + spear13xx_pcie_enable_interrupts(pp); > > +} > > + > > +static struct pcie_host_ops spear13xx_pcie_host_ops = { > > + .link_up = spear13xx_pcie_link_up, > > + .host_init = spear13xx_pcie_host_init, }; > > + > > +static int add_pcie_port(struct pcie_port *pp, struct platform_device > > +*pdev) { > > + struct device *dev = &pdev->dev; > > + int ret; > > + > > + pp->irq = platform_get_irq(pdev, 0); > > + if (!pp->irq) { > > + dev_err(dev, "failed to get irq\n"); > > + return -ENODEV; > > + } > > + ret = devm_request_irq(dev, pp->irq, spear13xx_pcie_irq_handler, > > + IRQF_SHARED, "spear1340-pcie", pp); > > + if (ret) { > > + dev_err(dev, "failed to request irq\n"); > > Please include the IRQ number in this message. > > > + return ret; > > + } > > + > > + pp->root_bus_nr = -1; > > + pp->ops = &spear13xx_pcie_host_ops; > > + > > + spin_lock_init(&pp->conf_lock); > > + ret = dw_pcie_host_init(pp); > > + if (ret) { > > + dev_err(dev, "failed to initialize host\n"); > > + return ret; > > + } > > + > > + return 0; > > +} > > + > > +static int __init spear13xx_pcie_probe(struct platform_device *pdev) > > +{ > > + struct spear13xx_pcie *spear13xx_pcie; > > + struct pcie_port *pp; > > + struct device *dev = &pdev->dev; > > + struct device_node *np = pdev->dev.of_node; > > + struct resource *dbi_base; > > + int ret; > > + > > + spear13xx_pcie = devm_kzalloc(dev, sizeof(*spear13xx_pcie), > > + GFP_KERNEL); > > + if (!spear13xx_pcie) { > > + dev_err(dev, "no memory for SPEAr13xx pcie\n"); > > + return -ENOMEM; > > + } > > + > > + spear13xx_pcie->phy = devm_phy_get(dev, "pcie-phy"); > > + if (IS_ERR(spear13xx_pcie->phy)) { > > + ret = PTR_ERR(spear13xx_pcie->phy); > > + switch (ret) { > > + case -EPROBE_DEFER: > > + dev_info(dev, "probe deferred\n"); > > + return ret; > > + default: > > + dev_err(dev, "couldn't get pcie-phy\n"); > > + return ret; > > + } > > This would be slightly simpler (your choice): > > if (ret == -EPROBE_DEFER) > dev_info(...) > else > dev_err(...) > return ret; > > > + } > > + > > + phy_init(spear13xx_pcie->phy); > > + > > + spear13xx_pcie->clk = devm_clk_get(dev, NULL); > > + if (IS_ERR(spear13xx_pcie->clk)) { > > + dev_err(dev, "couldn't get clk for pcie\n"); > > + return PTR_ERR(spear13xx_pcie->clk); > > + } > > + ret = clk_prepare_enable(spear13xx_pcie->clk); > > + if (ret) { > > + dev_err(dev, "couldn't enable clk for pcie\n"); > > + return ret; > > + } > > + > > + pp = &spear13xx_pcie->pp; > > + > > + pp->dev = dev; > > + > > + dbi_base = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > + pp->dbi_base = devm_ioremap_resource(dev, dbi_base); > > + if (IS_ERR(pp->dbi_base)) { > > + dev_err(dev, "couldn't remap dbi base\n"); > > Please include the dbi_base address in the message. > > > + ret = PTR_ERR(pp->dbi_base); > > + goto fail_clk; > > + } > > + spear13xx_pcie->app_base = pp->dbi_base + 0x2000; > > + > > + if (of_property_read_bool(np, "st,pcie-is-gen1")) > > + spear13xx_pcie->is_gen1 = true; > > + > > + ret = add_pcie_port(pp, pdev); > > + if (ret < 0) > > + goto fail_clk; > > + > > + platform_set_drvdata(pdev, spear13xx_pcie); > > + return 0; > > + > > +fail_clk: > > + clk_disable_unprepare(spear13xx_pcie->clk); > > + > > + return ret; > > +} > > + > > +static int __exit spear13xx_pcie_remove(struct platform_device *pdev) > > +{ > > + struct spear13xx_pcie *spear13xx_pcie = > platform_get_drvdata(pdev); > > + > > + clk_disable_unprepare(spear13xx_pcie->clk); > > + > > + phy_exit(spear13xx_pcie->phy); > > + > > + return 0; > > +} > > + > > +static const struct of_device_id spear13xx_pcie_of_match[] = { > > + { .compatible = "st,spear1340-pcie", }, > > + {}, > > +}; > > +MODULE_DEVICE_TABLE(of, spear13xx_pcie_of_match); > > + > > +static struct platform_driver spear13xx_pcie_driver = { > > + .probe = spear13xx_pcie_probe, > > + .remove = spear13xx_pcie_remove, > > + .driver = { > > + .name = "spear-pcie", > > + .owner = THIS_MODULE, > > + .of_match_table = > of_match_ptr(spear13xx_pcie_of_match), > > + }, > > +}; > > + > > +/* SPEAr13xx PCIe driver does not allow module unload */ > > + > > +static int __init pcie_init(void) > > +{ > > + return platform_driver_register(&spear13xx_pcie_driver); > > +} > > +module_init(pcie_init); > > + > > +MODULE_DESCRIPTION("ST Microelectronics SPEAr13xx PCIe host > > +controller driver"); MODULE_AUTHOR("Pratyush Anand > > +<pratyush.anand@xxxxxx>"); MODULE_LICENSE("GPL v2"); > > -- > > 1.7.0.1 > > > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-pci" > > in the body of a message to majordomo@xxxxxxxxxxxxxxx More > majordomo > > info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
