| Message ID | 1409938782-31460-1-git-send-email-m-karicheri2@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Friday 05 September 2014 13:39:42 Murali Karicheri wrote: > + > /* enable RC mode in devcfg */ > val = readl(reg_p); > - val &= ~PCIE_MODE_MASK; > - val |= PCIE_RC_MODE; > + port_id <<= 1; > + val &= ~(PCIE_MODE_MASK << port_id); > + val |= (PCIE_RC_MODE << port_id); > writel(val, reg_p); > + devm_iounmap(dev, reg_p); > + devm_release_mem_region(dev, res->start, resource_size(res)); This looks like it's a shared register of some sort that doesn't really belong into the registers of a particular port. Could it be that it's actually for the PHY? Arnd
On 09/05/2014 01:54 PM, Arnd Bergmann wrote: > On Friday 05 September 2014 13:39:42 Murali Karicheri wrote: >> + >> /* enable RC mode in devcfg */ >> val = readl(reg_p); >> - val&= ~PCIE_MODE_MASK; >> - val |= PCIE_RC_MODE; >> + port_id<<= 1; >> + val&= ~(PCIE_MODE_MASK<< port_id); >> + val |= (PCIE_RC_MODE<< port_id); >> writel(val, reg_p); >> + devm_iounmap(dev, reg_p); >> + devm_release_mem_region(dev, res->start, resource_size(res)); > > This looks like it's a shared register of some sort that doesn't > really belong into the registers of a particular port. Could it > be that it's actually for the PHY? > > Arnd Arnd, This a shared device configuration register between the two ports the desciption states it is bootstrap configuration of the PCIe module as Endpoint or Root complex and Not Phy. Hope below text will help. Table 3-23 Device Configuration Register (DEVCFG) PCIESSMODE[1:0] 00b PCIESSMODE is used to control the functionality of PCIESS module out of reset. This MMR output is connected to DEVTYPE input of PCIESS (Changes from Nysh) : Note that in Nysh this value came from a bootstrap pin. 00 : Endpoint 01 : Legacy Endpoint 10 : Rootcomplex 11 : Reserved PCIESS_1_MODE[1:0 ] 00b PCIESSMODE is used to control the functionality of PCIE_1 module out of reset. This MMR output is connected to DEVTYPE input of PCIE_1 00 : Endpoint 01 : Legacy Endpoint 10 : Rootcomplex 11 : Reserv
On Friday 05 September 2014 14:33:54 Murali Karicheri wrote: > > This looks like it's a shared register of some sort that doesn't > > really belong into the registers of a particular port. Could it > > be that it's actually for the PHY? > > > This a shared device configuration register between the two ports the > desciption states it is bootstrap configuration of the PCIe module as > Endpoint or Root complex and Not Phy. Hope below text will help. Ok. Why do you want to have this user-selectable though? Can't it just be set by the boot loader before starting Linux? Arnd > Table 3-23 Device Configuration Register (DEVCFG) > > > PCIESSMODE[1:0] 00b PCIESSMODE is used to control the > functionality of PCIESS module out of > reset. This MMR output is connected to > DEVTYPE input of PCIESS > (Changes from > Nysh) : Note that in Nysh this value came > from a bootstrap pin. > 00 : Endpoint > 01 : Legacy Endpoint > 10 : Rootcomplex > 11 : Reserved > PCIESS_1_MODE[1:0 > ] > 00b PCIESSMODE is used to control the > functionality of PCIE_1 module out of > reset. This MMR output is connected to > DEVTYPE input of PCIE_1 > 00 : Endpoint > 01 : Legacy Endpoint > 10 : Rootcomplex > 11 : Reserv >
On 09/05/2014 03:00 PM, Arnd Bergmann wrote: > On Friday 05 September 2014 14:33:54 Murali Karicheri wrote: >>> This looks like it's a shared register of some sort that doesn't >>> really belong into the registers of a particular port. Could it >>> be that it's actually for the PHY? >>> >> This a shared device configuration register between the two ports the >> desciption states it is bootstrap configuration of the PCIe module as >> Endpoint or Root complex and Not Phy. Hope below text will help. > > Ok. Why do you want to have this user-selectable though? Can't it > just be set by the boot loader before starting Linux? Arnd, As the driver is responsible for configuring the device to support the device functionality, it make sense to do this in the device driver. The driver enables clock to the IP and this is an addition thing to be configured so that when the device is powered up, it should function as RC. The IP can be configured to work as Root Complex or Endpoint. So not sure why you want to me to move this functionality to boot loader. Murali > > Arnd > >> Table 3-23 Device Configuration Register (DEVCFG) >> >> >> PCIESSMODE[1:0] 00b PCIESSMODE is used to control the >> functionality of PCIESS module out of >> reset. This MMR output is connected to >> DEVTYPE input of PCIESS >> (Changes from >> Nysh) : Note that in Nysh this value came >> from a bootstrap pin. >> 00 : Endpoint >> 01 : Legacy Endpoint >> 10 : Rootcomplex >> 11 : Reserved >> PCIESS_1_MODE[1:0 >> ] >> 00b PCIESSMODE is used to control the >> functionality of PCIE_1 module out of >> reset. This MMR output is connected to >> DEVTYPE input of PCIE_1 >> 00 : Endpoint >> 01 : Legacy Endpoint >> 10 : Rootcomplex >> 11 : Reserv >> >
On Friday 05 September 2014 16:37:25 Murali Karicheri wrote: > On 09/05/2014 03:00 PM, Arnd Bergmann wrote: > > On Friday 05 September 2014 14:33:54 Murali Karicheri wrote: > >>> This looks like it's a shared register of some sort that doesn't > >>> really belong into the registers of a particular port. Could it > >>> be that it's actually for the PHY? > >>> > >> This a shared device configuration register between the two ports the > >> desciption states it is bootstrap configuration of the PCIe module as > >> Endpoint or Root complex and Not Phy. Hope below text will help. > > > > Ok. Why do you want to have this user-selectable though? Can't it > > just be set by the boot loader before starting Linux? > > Arnd, > > As the driver is responsible for configuring the device to support the > device functionality, it make sense to do this in the device driver. The > driver enables clock to the IP and this is an addition thing to be > configured so that when the device is powered up, it should function as > RC. The IP can be configured to work as Root Complex or Endpoint. So not > sure why you want to me to move this functionality to boot loader. But the driver can only do root complex mode, and we would probably want a completely different driver if we were to start supporting endpoint mode. This also implies that the firmware has to pass a different DT for endpoint mode, so it should be responsible for setting up the hardware to match the DT. Arnd
On 09/05/2014 05:11 PM, Arnd Bergmann wrote: > On Friday 05 September 2014 16:37:25 Murali Karicheri wrote: >> On 09/05/2014 03:00 PM, Arnd Bergmann wrote: >>> On Friday 05 September 2014 14:33:54 Murali Karicheri wrote: >>>>> This looks like it's a shared register of some sort that doesn't >>>>> really belong into the registers of a particular port. Could it >>>>> be that it's actually for the PHY? >>>>> >>>> This a shared device configuration register between the two ports the >>>> desciption states it is bootstrap configuration of the PCIe module as >>>> Endpoint or Root complex and Not Phy. Hope below text will help. >>> >>> Ok. Why do you want to have this user-selectable though? Can't it >>> just be set by the boot loader before starting Linux? >> >> Arnd, >> >> As the driver is responsible for configuring the device to support the >> device functionality, it make sense to do this in the device driver. The >> driver enables clock to the IP and this is an addition thing to be >> configured so that when the device is powered up, it should function as >> RC. The IP can be configured to work as Root Complex or Endpoint. So not >> sure why you want to me to move this functionality to boot loader. > > But the driver can only do root complex mode, and we would probably > want a completely different driver if we were to start supporting > endpoint mode. > Arnd, Good point! I will drop index#2 handling in the driver code and will handle the same in boot loader. But I have a question though. The original driver which is queued up for merge to v3.18 has index #2 for this reg offset and is documented in the DT documentation as index 2 is the base address and length of PCI mode configuration register. index 3 is the base address and length of PCI device ID register. Will this create any issue in terms of backward compatibility if I remove it and move index3 to index2 and update the code for the same? I assume since this patch also will likely be on the next branch soon, and gets merged together with original driver to v3.18, this should be fine. But for some reason, if this patch doesn't make to v3.18, then won't this break the backward compatibility? I think the other option is document index2 as obsolete and update the document and remove the code for handling it. Any suggestion? Thanks Murali > This also implies that the firmware has to pass a different DT for > endpoint mode, so it should be responsible for setting up the hardware > to match the DT. > > Arnd > -- > To unsubscribe from this list: send the line "unsubscribe linux-pci" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On Monday 08 September 2014 11:52:47 Murali Karicheri wrote: > On 09/05/2014 05:11 PM, Arnd Bergmann wrote: > > But the driver can only do root complex mode, and we would probably > > want a completely different driver if we were to start supporting > > endpoint mode. > > Good point! I will drop index#2 handling in the driver code and will > handle the same in boot loader. But I have a question though. The > original driver which is queued up for merge to v3.18 has index #2 for > this reg offset and is documented in the DT documentation as > > index 2 is the base address and length of PCI mode configuration > register. > index 3 is the base address and length of PCI device ID register. > > > Will this create any issue in terms of backward compatibility if I > remove it and move index3 to index2 and update the code for the same? I > assume since this patch also will likely be on the next branch soon, and > gets merged together with original driver to v3.18, this should be fine. > But for some reason, if this patch doesn't make to v3.18, then won't > this break the backward compatibility? > > I think the other option is document index2 as obsolete and update the > document and remove the code for handling it. Any suggestion? Since the driver is not merged into Linus' tree yet and (more importantly) has not been in a release yet, we can still fix it. Please just send a patch on top to remove it now, it's not too late yet. Arnd
diff --git a/Documentation/devicetree/bindings/pci/pci-keystone.txt b/Documentation/devicetree/bindings/pci/pci-keystone.txt index ceb3e24..e279143 100644 --- a/Documentation/devicetree/bindings/pci/pci-keystone.txt +++ b/Documentation/devicetree/bindings/pci/pci-keystone.txt @@ -58,11 +58,13 @@ Optional properties:- phy-names: name of the Generic Keystine SerDes phy for PCI - If boot loader already does PCI link establishment, then phys and phy-names shouldn't be present. + ti,pcie-port: PCI port number. This is used to configure the PCI port + number. For example K2E SoC supports 2 PCI ports and PCI bindings + for the second port adds ti,pcie-port = <1> to identify second port + and driver uses this to configure the PCI mode register for second + port. If not present, port number 0 is assumed. Designware DT Properties not applicable for Keystone PCI 1. pcie_bus clock-names not used. Instead, a phandle to phys is used. -Note for PCI driver usage -========================= -Driver requires pci=pcie_bus_perf in the bootargs for proper functioning. diff --git a/drivers/pci/host/pci-keystone.c b/drivers/pci/host/pci-keystone.c index a132622..b4831e8 100644 --- a/drivers/pci/host/pci-keystone.c +++ b/drivers/pci/host/pci-keystone.c @@ -252,8 +252,8 @@ static int keystone_pcie_fault(unsigned long addr, unsigned int fsr, static void __init ks_pcie_host_init(struct pcie_port *pp) { - u32 vendor_device_id, val; struct keystone_pcie *ks_pcie = to_keystone_pcie(pp); + u32 val; ks_pcie_establish_link(ks_pcie); ks_dw_pcie_setup_rc_app_regs(ks_pcie); @@ -262,8 +262,7 @@ static void __init ks_pcie_host_init(struct pcie_port *pp) pp->dbi_base + PCI_IO_BASE); /* update the Vendor ID */ - vendor_device_id = readl(ks_pcie->va_reg_pciid); - writew((vendor_device_id >> 16), pp->dbi_base + PCI_DEVICE_ID); + writew(ks_pcie->device_id, pp->dbi_base + PCI_DEVICE_ID); /* update the DEV_STAT_CTRL to publish right mrrs */ val = readl(pp->dbi_base + PCIE_CAP_BASE + PCI_EXP_DEVCTL); @@ -344,12 +343,13 @@ static int __exit ks_pcie_remove(struct platform_device *pdev) static int __init ks_pcie_probe(struct platform_device *pdev) { struct device *dev = &pdev->dev; + struct device_node *np = dev->of_node; struct keystone_pcie *ks_pcie; + int ret = 0, port_id = 0; struct pcie_port *pp; struct resource *res; void __iomem *reg_p; struct phy *phy; - int ret = 0; u32 val; ks_pcie = devm_kzalloc(&pdev->dev, sizeof(*ks_pcie), @@ -360,17 +360,24 @@ static int __init ks_pcie_probe(struct platform_device *pdev) } pp = &ks_pcie->pp; + /* read the PCI port id */ + of_property_read_u32(np, "ti,pcie-port", &port_id); + /* index 2 is the devcfg register for RC mode settings */ res = platform_get_resource(pdev, IORESOURCE_MEM, 2); reg_p = devm_ioremap_resource(dev, res); if (IS_ERR(reg_p)) return PTR_ERR(reg_p); + /* enable RC mode in devcfg */ val = readl(reg_p); - val &= ~PCIE_MODE_MASK; - val |= PCIE_RC_MODE; + port_id <<= 1; + val &= ~(PCIE_MODE_MASK << port_id); + val |= (PCIE_RC_MODE << port_id); writel(val, reg_p); + devm_iounmap(dev, reg_p); + devm_release_mem_region(dev, res->start, resource_size(res)); /* initialize SerDes Phy if present */ phy = devm_phy_get(dev, "pcie-phy"); @@ -385,7 +392,9 @@ static int __init ks_pcie_probe(struct platform_device *pdev) reg_p = devm_ioremap_resource(dev, res); if (IS_ERR(reg_p)) return PTR_ERR(reg_p); - ks_pcie->va_reg_pciid = reg_p; + ks_pcie->device_id = readl(reg_p) >> 16; + devm_iounmap(dev, reg_p); + devm_release_mem_region(dev, res->start, resource_size(res)); pp->dev = dev; platform_set_drvdata(pdev, ks_pcie); diff --git a/drivers/pci/host/pci-keystone.h b/drivers/pci/host/pci-keystone.h index 729ea7d..80cfa8e 100644 --- a/drivers/pci/host/pci-keystone.h +++ b/drivers/pci/host/pci-keystone.h @@ -19,8 +19,8 @@ struct keystone_pcie { struct clk *clk; struct pcie_port pp; - void __iomem *va_reg_pciid; - + /* PCI Device ID */ + u32 device_id; int num_legacy_host_irqs; int legacy_host_irqs[MAX_LEGACY_HOST_IRQS]; struct device_node *legacy_intc_np;
K2E SoC has two PCI ports. The SATA controller is connected to second PCI port (port 1). This patch enhances the driver to support multiple ports. Update the DT Documentation for the new attribute, ti,pcie-port and remove the note for bootargs as this is no longer needed. Signed-off-by: Murali Karicheri <m-karicheri2@ti.com> CC: Rob Herring <robh+dt@kernel.org> CC: Pawel Moll <pawel.moll@arm.com> CC: Mark Rutland <mark.rutland@arm.com> CC: Ian Campbell <ijc+devicetree@hellion.org.uk> CC: Kumar Gala <galak@codeaurora.org> CC: Bjorn Helgaas <bhelgaas@google.com> CC: Santosh Shilimkar <santosh.shilimkar@ti.com> --- .../devicetree/bindings/pci/pci-keystone.txt | 8 ++++--- drivers/pci/host/pci-keystone.c | 23 ++++++++++++++------ drivers/pci/host/pci-keystone.h | 4 ++-- 3 files changed, 23 insertions(+), 12 deletions(-)