| Message ID | 1410293835-13050-2-git-send-email-m-karicheri2@ti.com |
|---|---|
| State | Superseded |
| Headers | show |
On Tuesday 09 September 2014 16:17:15 Murali Karicheri wrote: > 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> > --- > - updated based on comment against v1 > .../devicetree/bindings/pci/pci-keystone.txt | 8 +++++--- > drivers/pci/host/pci-keystone.c | 15 ++++++++++----- > drivers/pci/host/pci-keystone.h | 4 ++-- > 3 files changed, 17 insertions(+), 10 deletions(-) > > diff --git a/Documentation/devicetree/bindings/pci/pci-keystone.txt b/Documentation/devicetree/bindings/pci/pci-keystone.txt > index bedacf0..c8f5773 100644 > --- a/Documentation/devicetree/bindings/pci/pci-keystone.txt > +++ b/Documentation/devicetree/bindings/pci/pci-keystone.txt > @@ -56,11 +56,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. Is this still needed? I thought with patch 1 removed, we no longer had a reason to know the port number. > -========================= > -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 f1119eb..893820d 100644 > --- a/drivers/pci/host/pci-keystone.c > +++ b/drivers/pci/host/pci-keystone.c > @@ -253,8 +253,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); > @@ -263,8 +263,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); This change must have slipped in accidentally, at least it's not described in the changelog. Should this be another patch? The change seems useful. 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 09/09/2014 04:28 PM, Arnd Bergmann wrote: > On Tuesday 09 September 2014 16:17:15 Murali Karicheri wrote: >> 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> >> --- >> - updated based on comment against v1 >> .../devicetree/bindings/pci/pci-keystone.txt | 8 +++++--- >> drivers/pci/host/pci-keystone.c | 15 ++++++++++----- >> drivers/pci/host/pci-keystone.h | 4 ++-- >> 3 files changed, 17 insertions(+), 10 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/pci/pci-keystone.txt b/Documentation/devicetree/bindings/pci/pci-keystone.txt >> index bedacf0..c8f5773 100644 >> --- a/Documentation/devicetree/bindings/pci/pci-keystone.txt >> +++ b/Documentation/devicetree/bindings/pci/pci-keystone.txt >> @@ -56,11 +56,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. > > Is this still needed? I thought with patch 1 removed, we no longer had > a reason to know the port number. > You are right. I will remove it and re-send. >> -========================= >> -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 f1119eb..893820d 100644 >> --- a/drivers/pci/host/pci-keystone.c >> +++ b/drivers/pci/host/pci-keystone.c >> @@ -253,8 +253,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); >> @@ -263,8 +263,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); > > This change must have slipped in accidentally, at least it's not > described in the changelog. Should this be another patch? > The change seems useful. Are you referring to mrrs or update to device id? device id is in a SoC register at index2 and is read and updated by the driver here. MRRS update was originally in the code. Murali > > 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 Tuesday 09 September 2014 16:42:46 Murali Karicheri wrote: > >> > >> /* 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); > > > > This change must have slipped in accidentally, at least it's not > > described in the changelog. Should this be another patch? > > The change seems useful. > Are you referring to mrrs or update to device id? device id is in a SoC > register at index2 and is read and updated by the driver here. MRRS > update was originally in the code. > I meant the device id change. Maybe you accidentally did 'git commit --amend' during a rebase and that replaced the real changelog with the one of the patch in front of it and merged the two patches? That happened to me a few times and would explain the strange mix of two changes. 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 09/09/2014 05:09 PM, Arnd Bergmann wrote: > On Tuesday 09 September 2014 16:42:46 Murali Karicheri wrote: >>>> >>>> /* 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); >>> >>> This change must have slipped in accidentally, at least it's not >>> described in the changelog. Should this be another patch? >>> The change seems useful. >> Are you referring to mrrs or update to device id? device id is in a SoC >> register at index2 and is read and updated by the driver here. MRRS >> update was originally in the code. >> > > I meant the device id change. Maybe you accidentally did 'git commit > --amend' during a rebase and that replaced the real changelog with > the one of the patch in front of it and merged the two patches? > Actually this is an inteded. The vendor ID is in a register indicated by reg offset and as per the device spec, it needs to be read and updated by the software. Now since multiple instances of PCI device needs to be read the same register, the reading happens in the probe() and same is unmapped after that. + ks_pcie->device_id = readl(reg_p) >> 16; + devm_iounmap(dev, reg_p); + devm_release_mem_region(dev, res->start, resource_size(res)); Afetr that in ks_pcie_host_init(), it update the device_id in the RC's config space. Thanks Murali > That happened to me a few times and would explain the strange mix > of two changes. > > 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 09/09/2014 05:49 PM, Murali Karicheri wrote: > On 09/09/2014 05:09 PM, Arnd Bergmann wrote: >> On Tuesday 09 September 2014 16:42:46 Murali Karicheri wrote: >>>>> >>>>> /* 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); >>>> >>>> This change must have slipped in accidentally, at least it's not >>>> described in the changelog. Should this be another patch? >>>> The change seems useful. >>> Are you referring to mrrs or update to device id? device id is in a SoC >>> register at index2 and is read and updated by the driver here. MRRS >>> update was originally in the code. >>> >> >> I meant the device id change. Maybe you accidentally did 'git commit >> --amend' during a rebase and that replaced the real changelog with >> the one of the patch in front of it and merged the two patches? >> > Actually this is an inteded. The vendor ID is in a register indicated by > reg offset and as per the device spec, it needs to be read and updated > by the software. Now since multiple instances of PCI device needs to be > read the same register, the reading happens in the probe() and same is > unmapped after that. > > + ks_pcie->device_id = readl(reg_p) >> 16; > + devm_iounmap(dev, reg_p); > + devm_release_mem_region(dev, res->start, resource_size(res)); > > Afetr that in ks_pcie_host_init(), it update the device_id in the RC's > config space. > BTW, I will update the commit log with more description to indicate the above and re-send it if this is fine. > Thanks > > Murali >> That happened to me a few times and would explain the strange mix >> of two changes. >> >> 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 Tuesday 09 September 2014 17:49:19 Murali Karicheri wrote: > Actually this is an inteded. The vendor ID is in a register indicated by > reg offset and as per the device spec, it needs to be read and updated > by the software. Now since multiple instances of PCI device needs to be > read the same register, the reading happens in the probe() and same is > unmapped after that. > > + ks_pcie->device_id = readl(reg_p) >> 16; > + devm_iounmap(dev, reg_p); > + devm_release_mem_region(dev, res->start, resource_size(res)); > > Afetr that in ks_pcie_host_init(), it update the device_id in the RC's > config space. I'm not sure I understand the purpose of this. Do you mean you read the vendor/device ID of whichever device happens to get probed first and then program the same ID into all other devices as well? What if the order changes between two boots? Why does the vendor/device ID of the host bridge even matter at all? 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 09/09/2014 05:52 PM, Arnd Bergmann wrote: > On Tuesday 09 September 2014 17:49:19 Murali Karicheri wrote: >> Actually this is an inteded. The vendor ID is in a register indicated by >> reg offset and as per the device spec, it needs to be read and updated >> by the software. Now since multiple instances of PCI device needs to be >> read the same register, the reading happens in the probe() and same is >> unmapped after that. >> >> + ks_pcie->device_id = readl(reg_p)>> 16; >> + devm_iounmap(dev, reg_p); >> + devm_release_mem_region(dev, res->start, resource_size(res)); >> >> Afetr that in ks_pcie_host_init(), it update the device_id in the RC's >> config space. > > I'm not sure I understand the purpose of this. Do you mean you read > the vendor/device ID of whichever device happens to get probed first > and then program the same ID into all other devices as well? My mistake. It is the device ID, not vendor ID. The PCI driver supports PCI h/w on K2HK, K2E and K2L SoCs for which PCI device IDs are assigned as +#define PCIE_RC_K2HK 0xb008 +#define PCIE_RC_K2E 0xb009 +#define PCIE_RC_K2L 0xb00a + and the same driver code runs on all these h/w. The device ID is not filled in by default by the h/w, in the config space of the RC at offset 1000h "VENDOR_DEVICE_ID Vendor and Device Identification Register". Same is available in a seperate SoC register whose offset is specified by index 2. This is read by driver and updated in the config space. The Vendor ID is however set by default. There is a mrrs PCI quirk required for Keystone PCI which depends on this vendor ID to be filled correctly as it match vendor id/ device id of the bridge device to apply the quirk. Hope this clarify your question. Murali > > What if the order changes between two boots? Why does the vendor/device > ID of the host bridge even matter at all? > > 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 Tuesday 09 September 2014 18:50:13 Murali Karicheri wrote: > My mistake. It is the device ID, not vendor ID. The PCI driver supports > PCI h/w on K2HK, K2E and K2L SoCs for which PCI device IDs are assigned as > > +#define PCIE_RC_K2HK 0xb008 > +#define PCIE_RC_K2E 0xb009 > +#define PCIE_RC_K2L 0xb00a > + > > and the same driver code runs on all these h/w. The device ID is not > filled in by default by the h/w, in the config space of the RC at offset > 1000h "VENDOR_DEVICE_ID Vendor and Device Identification Register". > Same is available in a seperate SoC register whose offset is specified > by index 2. This is read by driver and updated in the config space. The > Vendor ID is however set by default. > > There is a mrrs PCI quirk required for Keystone PCI which depends on > this vendor ID to be filled correctly as it match vendor id/ device id > of the bridge device to apply the quirk. > > Hope this clarify your question. Ok, I understand now. Yes, this makes sense, though I wonder if it would have been easier to handle the quirk in a different way based on the driver rather than the PCI ID. It's probably not worth revisiting though, unless Bjorn wants to see it done differently now. 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 09/10/2014 04:22 AM, Arnd Bergmann wrote: > On Tuesday 09 September 2014 18:50:13 Murali Karicheri wrote: >> My mistake. It is the device ID, not vendor ID. The PCI driver supports >> PCI h/w on K2HK, K2E and K2L SoCs for which PCI device IDs are assigned as >> >> +#define PCIE_RC_K2HK 0xb008 >> +#define PCIE_RC_K2E 0xb009 >> +#define PCIE_RC_K2L 0xb00a >> + >> >> and the same driver code runs on all these h/w. The device ID is not >> filled in by default by the h/w, in the config space of the RC at offset >> 1000h "VENDOR_DEVICE_ID Vendor and Device Identification Register". >> Same is available in a seperate SoC register whose offset is specified >> by index 2. This is read by driver and updated in the config space. The >> Vendor ID is however set by default. >> >> There is a mrrs PCI quirk required for Keystone PCI which depends on >> this vendor ID to be filled correctly as it match vendor id/ device id >> of the bridge device to apply the quirk. >> >> Hope this clarify your question. > > Ok, I understand now. Yes, this makes sense, though I wonder if it would > have been easier to handle the quirk in a different way based on the > driver rather than the PCI ID. It's probably not worth revisiting though, > unless Bjorn wants to see it done differently now. Bjorn has reviewed the quirk patch and wanted to check the device ID and vendor ID to avoid applying the quirck when bridge is non TI and same device ID. So support for two PCI port requires only the PCI device id related change. So I will modify the subject and commit log to reflect the same and re-send the patch. Thanks again Arnd for your comments. Regards, Murali > > 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 -- 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
diff --git a/Documentation/devicetree/bindings/pci/pci-keystone.txt b/Documentation/devicetree/bindings/pci/pci-keystone.txt index bedacf0..c8f5773 100644 --- a/Documentation/devicetree/bindings/pci/pci-keystone.txt +++ b/Documentation/devicetree/bindings/pci/pci-keystone.txt @@ -56,11 +56,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 f1119eb..893820d 100644 --- a/drivers/pci/host/pci-keystone.c +++ b/drivers/pci/host/pci-keystone.c @@ -253,8 +253,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); @@ -263,8 +263,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); @@ -345,12 +344,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; ks_pcie = devm_kzalloc(&pdev->dev, sizeof(*ks_pcie), GFP_KERNEL); @@ -360,6 +360,9 @@ 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); + /* initialize SerDes Phy if present */ phy = devm_phy_get(dev, "pcie-phy"); if (!IS_ERR_OR_NULL(phy)) { @@ -373,7 +376,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> --- - updated based on comment against v1 .../devicetree/bindings/pci/pci-keystone.txt | 8 +++++--- drivers/pci/host/pci-keystone.c | 15 ++++++++++----- drivers/pci/host/pci-keystone.h | 4 ++-- 3 files changed, 17 insertions(+), 10 deletions(-)