| Message ID | 20200430185522.4116-3-james.quinlan@broadcom.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/5] PCI: brcmstb: don't clk_put() a managed clock | expand |
On 4/30/20 11:55 AM, Jim Quinlan wrote: > From: Jim Quinlan <jquinlan@broadcom.com> > > Configuration Retry Request Status is off by default on this > PCIe controller. Turn it on. > > Signed-off-by: Jim Quinlan <jquinlan@broadcom.com> Acked-by: Florian Fainelli <f.fainelli@gmail.com>
On Thu, Apr 30, 2020 at 02:55:20PM -0400, Jim Quinlan wrote: > From: Jim Quinlan <jquinlan@broadcom.com> > > Configuration Retry Request Status is off by default on this > PCIe controller. Turn it on. Are you talking about CRS itself, i.e., the ability of a Root Port to deal with Completions with Configuration Retry Request Status? That really shouldn't be switchable in the hardware since it's a required feature for all PCIe devices. Or are you talking about CRS Software Visibility, which is controlled by a bit in the PCIe Root Control register? That *should* be managed by the PCI core in pci_enable_crs(). Does that generic method of controlling it not work for this device? It looks like maybe the latter, since the generic: #define PCI_EXP_RTCTL_CRSSVE 0x0010 matches your new PCIE_RC_CFG_PCIE_ROOT_CAP_CONTROL_RC_CRS_EN_MASK. If pci_enable_crs() doesn't work on this device, it sounds like a hardware defect that we need to work around, but I'm not sure that just enabling it unconditionally here is the right thing. > Signed-off-by: Jim Quinlan <jquinlan@broadcom.com> > --- > drivers/pci/controller/pcie-brcmstb.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c > index 5b0dec5971b8..2bc913c0262c 100644 > --- a/drivers/pci/controller/pcie-brcmstb.c > +++ b/drivers/pci/controller/pcie-brcmstb.c > @@ -34,6 +34,9 @@ > #define BRCM_PCIE_CAP_REGS 0x00ac > > /* Broadcom STB PCIe Register Offsets */ > +#define PCIE_RC_CFG_PCIE_ROOT_CAP_CONTROL 0x00c8 > +#define PCIE_RC_CFG_PCIE_ROOT_CAP_CONTROL_RC_CRS_EN_MASK 0x10 > + > #define PCIE_RC_CFG_VENDOR_VENDOR_SPECIFIC_REG1 0x0188 > #define PCIE_RC_CFG_VENDOR_VENDOR_SPECIFIC_REG1_ENDIAN_MODE_BAR2_MASK 0xc > #define PCIE_RC_CFG_VENDOR_SPCIFIC_REG1_LITTLE_ENDIAN 0x0 > @@ -827,6 +830,12 @@ static int brcm_pcie_setup(struct brcm_pcie *pcie) > pci_speed_string(pcie_link_speed[cls]), nlw, > ssc_good ? "(SSC)" : "(!SSC)"); > > + /* Enable configuration request retry (CRS) */ > + tmp = readl(base + PCIE_RC_CFG_PCIE_ROOT_CAP_CONTROL); > + u32p_replace_bits(&tmp, 1, > + PCIE_RC_CFG_PCIE_ROOT_CAP_CONTROL_RC_CRS_EN_MASK); > + writel(tmp, base + PCIE_RC_CFG_PCIE_ROOT_CAP_CONTROL); > + > /* PCIe->SCB endian mode for BAR */ > tmp = readl(base + PCIE_RC_CFG_VENDOR_VENDOR_SPECIFIC_REG1); > u32p_replace_bits(&tmp, PCIE_RC_CFG_VENDOR_SPCIFIC_REG1_LITTLE_ENDIAN, > -- > 2.17.1 >
On Thu, Apr 30, 2020 at 4:32 PM Bjorn Helgaas <helgaas@kernel.org> wrote: > > On Thu, Apr 30, 2020 at 02:55:20PM -0400, Jim Quinlan wrote: > > From: Jim Quinlan <jquinlan@broadcom.com> > > > > Configuration Retry Request Status is off by default on this > > PCIe controller. Turn it on. > > Are you talking about CRS itself, i.e., the ability of a Root Port to > deal with Completions with Configuration Retry Request Status? That > really shouldn't be switchable in the hardware since it's a required > feature for all PCIe devices. > > Or are you talking about CRS Software Visibility, which is controlled > by a bit in the PCIe Root Control register? That *should* be managed > by the PCI core in pci_enable_crs(). Does that generic method of > controlling it not work for this device? > My mistake; the commit will be dropped. Thanks, Jim > It looks like maybe the latter, since the generic: > > #define PCI_EXP_RTCTL_CRSSVE 0x0010 > > matches your new PCIE_RC_CFG_PCIE_ROOT_CAP_CONTROL_RC_CRS_EN_MASK. > > If pci_enable_crs() doesn't work on this device, it sounds like a > hardware defect that we need to work around, but I'm not sure that > just enabling it unconditionally here is the right thing. > > > Signed-off-by: Jim Quinlan <jquinlan@broadcom.com> > > --- > > drivers/pci/controller/pcie-brcmstb.c | 9 +++++++++ > > 1 file changed, 9 insertions(+) > > > > diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c > > index 5b0dec5971b8..2bc913c0262c 100644 > > --- a/drivers/pci/controller/pcie-brcmstb.c > > +++ b/drivers/pci/controller/pcie-brcmstb.c > > @@ -34,6 +34,9 @@ > > #define BRCM_PCIE_CAP_REGS 0x00ac > > > > /* Broadcom STB PCIe Register Offsets */ > > +#define PCIE_RC_CFG_PCIE_ROOT_CAP_CONTROL 0x00c8 > > +#define PCIE_RC_CFG_PCIE_ROOT_CAP_CONTROL_RC_CRS_EN_MASK 0x10 > > + > > #define PCIE_RC_CFG_VENDOR_VENDOR_SPECIFIC_REG1 0x0188 > > #define PCIE_RC_CFG_VENDOR_VENDOR_SPECIFIC_REG1_ENDIAN_MODE_BAR2_MASK 0xc > > #define PCIE_RC_CFG_VENDOR_SPCIFIC_REG1_LITTLE_ENDIAN 0x0 > > @@ -827,6 +830,12 @@ static int brcm_pcie_setup(struct brcm_pcie *pcie) > > pci_speed_string(pcie_link_speed[cls]), nlw, > > ssc_good ? "(SSC)" : "(!SSC)"); > > > > + /* Enable configuration request retry (CRS) */ > > + tmp = readl(base + PCIE_RC_CFG_PCIE_ROOT_CAP_CONTROL); > > + u32p_replace_bits(&tmp, 1, > > + PCIE_RC_CFG_PCIE_ROOT_CAP_CONTROL_RC_CRS_EN_MASK); > > + writel(tmp, base + PCIE_RC_CFG_PCIE_ROOT_CAP_CONTROL); > > + > > /* PCIe->SCB endian mode for BAR */ > > tmp = readl(base + PCIE_RC_CFG_VENDOR_VENDOR_SPECIFIC_REG1); > > u32p_replace_bits(&tmp, PCIE_RC_CFG_VENDOR_SPCIFIC_REG1_LITTLE_ENDIAN, > > -- > > 2.17.1 > >
diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c index 5b0dec5971b8..2bc913c0262c 100644 --- a/drivers/pci/controller/pcie-brcmstb.c +++ b/drivers/pci/controller/pcie-brcmstb.c @@ -34,6 +34,9 @@ #define BRCM_PCIE_CAP_REGS 0x00ac /* Broadcom STB PCIe Register Offsets */ +#define PCIE_RC_CFG_PCIE_ROOT_CAP_CONTROL 0x00c8 +#define PCIE_RC_CFG_PCIE_ROOT_CAP_CONTROL_RC_CRS_EN_MASK 0x10 + #define PCIE_RC_CFG_VENDOR_VENDOR_SPECIFIC_REG1 0x0188 #define PCIE_RC_CFG_VENDOR_VENDOR_SPECIFIC_REG1_ENDIAN_MODE_BAR2_MASK 0xc #define PCIE_RC_CFG_VENDOR_SPCIFIC_REG1_LITTLE_ENDIAN 0x0 @@ -827,6 +830,12 @@ static int brcm_pcie_setup(struct brcm_pcie *pcie) pci_speed_string(pcie_link_speed[cls]), nlw, ssc_good ? "(SSC)" : "(!SSC)"); + /* Enable configuration request retry (CRS) */ + tmp = readl(base + PCIE_RC_CFG_PCIE_ROOT_CAP_CONTROL); + u32p_replace_bits(&tmp, 1, + PCIE_RC_CFG_PCIE_ROOT_CAP_CONTROL_RC_CRS_EN_MASK); + writel(tmp, base + PCIE_RC_CFG_PCIE_ROOT_CAP_CONTROL); + /* PCIe->SCB endian mode for BAR */ tmp = readl(base + PCIE_RC_CFG_VENDOR_VENDOR_SPECIFIC_REG1); u32p_replace_bits(&tmp, PCIE_RC_CFG_VENDOR_SPCIFIC_REG1_LITTLE_ENDIAN,