| Message ID | 20241014130710.413-7-svarbanov@suse.de |
|---|---|
| State | New |
| Headers | show |
| Series | Add PCIe support for bcm2712 | expand |
On Mon, Oct 14, 2024 at 04:07:05PM +0300, Stanimir Varbanov wrote: > On PCIe turn off avoid shutdown of bridge reset, > by introducing a quirk flag. Can you include something here about *why* we need this change? I think the RESCAL comment below would be a good start. I think this should be squashed with the next commit that adds the use of CFG_QUIRK_AVOID_BRIDGE_SHUTDOWN. Otherwise this commit doesn't have an obvious reason. > Signed-off-by: Stanimir Varbanov <svarbanov@suse.de> > --- > v2 -> v3: > - Added more descriptive comment on CFG_QUIRK_AVOID_BRIDGE_SHUTDOWN quirk. > > drivers/pci/controller/pcie-brcmstb.c | 17 +++++++++++++++-- > 1 file changed, 15 insertions(+), 2 deletions(-) > > diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c > index b76c16287f37..757a1646d53c 100644 > --- a/drivers/pci/controller/pcie-brcmstb.c > +++ b/drivers/pci/controller/pcie-brcmstb.c > @@ -234,10 +234,20 @@ struct inbound_win { > u64 cpu_addr; > }; > > +/* > + * The RESCAL block is tied to PCIe controller #1, regardless of the number of > + * controllers, and turning off PCIe controller #1 prevents access to the RESCAL > + * register blocks, therefore not other controller can access this register > + * space, and depending upon the bus fabric we may get a timeout (UBUS/GISB), > + * or a hang (AXI). s/not other/no other/ > + */ > +#define CFG_QUIRK_AVOID_BRIDGE_SHUTDOWN BIT(0) > + > struct pcie_cfg_data { > const int *offsets; > const enum pcie_soc_base soc_base; > const bool has_phy; > + const u32 quirks; > u8 num_inbound_wins; > int (*perst_set)(struct brcm_pcie *pcie, u32 val); > int (*bridge_sw_init_set)(struct brcm_pcie *pcie, u32 val); > @@ -290,6 +300,7 @@ struct brcm_pcie { > struct subdev_regulators *sr; > bool ep_wakeup_capable; > bool has_phy; > + u32 quirks; > u8 num_inbound_wins; > }; > > @@ -1539,8 +1550,9 @@ static int brcm_pcie_turn_off(struct brcm_pcie *pcie) > u32p_replace_bits(&tmp, 1, PCIE_MISC_HARD_PCIE_HARD_DEBUG_SERDES_IDDQ_MASK); > writel(tmp, base + HARD_DEBUG(pcie)); > > - /* Shutdown PCIe bridge */ > - ret = pcie->bridge_sw_init_set(pcie, 1); > + if (!(pcie->quirks & CFG_QUIRK_AVOID_BRIDGE_SHUTDOWN)) > + /* Shutdown PCIe bridge */ > + ret = pcie->bridge_sw_init_set(pcie, 1); > > return ret; > } > @@ -1854,6 +1866,7 @@ static int brcm_pcie_probe(struct platform_device *pdev) > pcie->perst_set = data->perst_set; > pcie->bridge_sw_init_set = data->bridge_sw_init_set; > pcie->has_phy = data->has_phy; > + pcie->quirks = data->quirks; > pcie->num_inbound_wins = data->num_inbound_wins; > > pcie->base = devm_platform_ioremap_resource(pdev, 0); > -- > 2.43.0 >
On 10/14/24 10:01, Bjorn Helgaas wrote: > On Mon, Oct 14, 2024 at 04:07:05PM +0300, Stanimir Varbanov wrote: >> On PCIe turn off avoid shutdown of bridge reset, >> by introducing a quirk flag. > > Can you include something here about *why* we need this change? I > think the RESCAL comment below would be a good start. > > I think this should be squashed with the next commit that adds the use > of CFG_QUIRK_AVOID_BRIDGE_SHUTDOWN. Otherwise this commit doesn't > have an obvious reason. Agreed.
On Mon, Oct 14, 2024 at 9:07 AM Stanimir Varbanov <svarbanov@suse.de> wrote: > > On PCIe turn off avoid shutdown of bridge reset, > by introducing a quirk flag. > > Signed-off-by: Stanimir Varbanov <svarbanov@suse.de> > --- > v2 -> v3: > - Added more descriptive comment on CFG_QUIRK_AVOID_BRIDGE_SHUTDOWN quirk. > > drivers/pci/controller/pcie-brcmstb.c | 17 +++++++++++++++-- > 1 file changed, 15 insertions(+), 2 deletions(-) > > diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c > index b76c16287f37..757a1646d53c 100644 > --- a/drivers/pci/controller/pcie-brcmstb.c > +++ b/drivers/pci/controller/pcie-brcmstb.c > @@ -234,10 +234,20 @@ struct inbound_win { > u64 cpu_addr; > }; > > +/* > + * The RESCAL block is tied to PCIe controller #1, regardless of the number of > + * controllers, and turning off PCIe controller #1 prevents access to the RESCAL > + * register blocks, therefore not other controller can access this register s/no/not/ I assume that the quirks is specific to 2712 as the 7712 does not need this since it only has PCIe1 (I'll probably seethis as I read more of your commits). -- Jim > + * space, and depending upon the bus fabric we may get a timeout (UBUS/GISB), > + * or a hang (AXI). > + */ > +#define CFG_QUIRK_AVOID_BRIDGE_SHUTDOWN BIT(0) > + > struct pcie_cfg_data { > const int *offsets; > const enum pcie_soc_base soc_base; > const bool has_phy; > + const u32 quirks; > u8 num_inbound_wins; > int (*perst_set)(struct brcm_pcie *pcie, u32 val); > int (*bridge_sw_init_set)(struct brcm_pcie *pcie, u32 val); > @@ -290,6 +300,7 @@ struct brcm_pcie { > struct subdev_regulators *sr; > bool ep_wakeup_capable; > bool has_phy; > + u32 quirks; > u8 num_inbound_wins; > }; > > @@ -1539,8 +1550,9 @@ static int brcm_pcie_turn_off(struct brcm_pcie *pcie) > u32p_replace_bits(&tmp, 1, PCIE_MISC_HARD_PCIE_HARD_DEBUG_SERDES_IDDQ_MASK); > writel(tmp, base + HARD_DEBUG(pcie)); > > - /* Shutdown PCIe bridge */ > - ret = pcie->bridge_sw_init_set(pcie, 1); > + if (!(pcie->quirks & CFG_QUIRK_AVOID_BRIDGE_SHUTDOWN)) > + /* Shutdown PCIe bridge */ > + ret = pcie->bridge_sw_init_set(pcie, 1); > > return ret; > } > @@ -1854,6 +1866,7 @@ static int brcm_pcie_probe(struct platform_device *pdev) > pcie->perst_set = data->perst_set; > pcie->bridge_sw_init_set = data->bridge_sw_init_set; > pcie->has_phy = data->has_phy; > + pcie->quirks = data->quirks; > pcie->num_inbound_wins = data->num_inbound_wins; > > pcie->base = devm_platform_ioremap_resource(pdev, 0); > -- > 2.43.0 >
Hi Jim, On 10/16/24 20:17, Jim Quinlan wrote: > On Mon, Oct 14, 2024 at 9:07 AM Stanimir Varbanov <svarbanov@suse.de> wrote: >> >> On PCIe turn off avoid shutdown of bridge reset, >> by introducing a quirk flag. >> >> Signed-off-by: Stanimir Varbanov <svarbanov@suse.de> >> --- >> v2 -> v3: >> - Added more descriptive comment on CFG_QUIRK_AVOID_BRIDGE_SHUTDOWN quirk. >> >> drivers/pci/controller/pcie-brcmstb.c | 17 +++++++++++++++-- >> 1 file changed, 15 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c >> index b76c16287f37..757a1646d53c 100644 >> --- a/drivers/pci/controller/pcie-brcmstb.c >> +++ b/drivers/pci/controller/pcie-brcmstb.c >> @@ -234,10 +234,20 @@ struct inbound_win { >> u64 cpu_addr; >> }; >> >> +/* >> + * The RESCAL block is tied to PCIe controller #1, regardless of the number of >> + * controllers, and turning off PCIe controller #1 prevents access to the RESCAL >> + * register blocks, therefore not other controller can access this register > > s/no/not/ > > I assume that the quirks is specific to 2712 as the 7712 does not need > this since it only has PCIe1 > (I'll probably seethis as I read more of your commits). Yes, the .post_setup op is implemented for 2712 only. Look into next patch in the series. ~Stan
Hi, On 10/14/24 20:02, Florian Fainelli wrote: > On 10/14/24 10:01, Bjorn Helgaas wrote: >> On Mon, Oct 14, 2024 at 04:07:05PM +0300, Stanimir Varbanov wrote: >>> On PCIe turn off avoid shutdown of bridge reset, >>> by introducing a quirk flag. >> >> Can you include something here about *why* we need this change? I >> think the RESCAL comment below would be a good start. >> >> I think this should be squashed with the next commit that adds the use >> of CFG_QUIRK_AVOID_BRIDGE_SHUTDOWN. Otherwise this commit doesn't >> have an obvious reason. > > Agreed. OK, will do. Thank you for the review! ~Stan
diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c index b76c16287f37..757a1646d53c 100644 --- a/drivers/pci/controller/pcie-brcmstb.c +++ b/drivers/pci/controller/pcie-brcmstb.c @@ -234,10 +234,20 @@ struct inbound_win { u64 cpu_addr; }; +/* + * The RESCAL block is tied to PCIe controller #1, regardless of the number of + * controllers, and turning off PCIe controller #1 prevents access to the RESCAL + * register blocks, therefore not other controller can access this register + * space, and depending upon the bus fabric we may get a timeout (UBUS/GISB), + * or a hang (AXI). + */ +#define CFG_QUIRK_AVOID_BRIDGE_SHUTDOWN BIT(0) + struct pcie_cfg_data { const int *offsets; const enum pcie_soc_base soc_base; const bool has_phy; + const u32 quirks; u8 num_inbound_wins; int (*perst_set)(struct brcm_pcie *pcie, u32 val); int (*bridge_sw_init_set)(struct brcm_pcie *pcie, u32 val); @@ -290,6 +300,7 @@ struct brcm_pcie { struct subdev_regulators *sr; bool ep_wakeup_capable; bool has_phy; + u32 quirks; u8 num_inbound_wins; }; @@ -1539,8 +1550,9 @@ static int brcm_pcie_turn_off(struct brcm_pcie *pcie) u32p_replace_bits(&tmp, 1, PCIE_MISC_HARD_PCIE_HARD_DEBUG_SERDES_IDDQ_MASK); writel(tmp, base + HARD_DEBUG(pcie)); - /* Shutdown PCIe bridge */ - ret = pcie->bridge_sw_init_set(pcie, 1); + if (!(pcie->quirks & CFG_QUIRK_AVOID_BRIDGE_SHUTDOWN)) + /* Shutdown PCIe bridge */ + ret = pcie->bridge_sw_init_set(pcie, 1); return ret; } @@ -1854,6 +1866,7 @@ static int brcm_pcie_probe(struct platform_device *pdev) pcie->perst_set = data->perst_set; pcie->bridge_sw_init_set = data->bridge_sw_init_set; pcie->has_phy = data->has_phy; + pcie->quirks = data->quirks; pcie->num_inbound_wins = data->num_inbound_wins; pcie->base = devm_platform_ioremap_resource(pdev, 0);
On PCIe turn off avoid shutdown of bridge reset, by introducing a quirk flag. Signed-off-by: Stanimir Varbanov <svarbanov@suse.de> --- v2 -> v3: - Added more descriptive comment on CFG_QUIRK_AVOID_BRIDGE_SHUTDOWN quirk. drivers/pci/controller/pcie-brcmstb.c | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-)