| Message ID | 20240628205430.24775-4-james.quinlan@broadcom.com |
|---|---|
| State | New |
| Headers | show |
| Series | PCI: brcnstb: Enable STB 7712 SOC | expand |
On 6/28/24 23:54, Jim Quinlan wrote: > The 7712 SOC has a bridge reset which can be described in the device tree. > If it is present, use it. Otherwise, continue to use the legacy method to > reset the bridge. > > Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com> > --- > drivers/pci/controller/pcie-brcmstb.c | 24 +++++++++++++++++++----- > 1 file changed, 19 insertions(+), 5 deletions(-) > > diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c > index c2eb29b886f7..4104c3668fdb 100644 > --- a/drivers/pci/controller/pcie-brcmstb.c > +++ b/drivers/pci/controller/pcie-brcmstb.c > @@ -265,6 +265,7 @@ struct brcm_pcie { > enum pcie_type type; > struct reset_control *rescal; > struct reset_control *perst_reset; > + struct reset_control *bridge; > int num_memc; > u64 memc_size[PCIE_BRCM_MAX_MEMC]; > u32 hw_rev; > @@ -732,12 +733,19 @@ static void __iomem *brcm7425_pcie_map_bus(struct pci_bus *bus, > > static void brcm_pcie_bridge_sw_init_set_generic(struct brcm_pcie *pcie, u32 val) > { > - u32 tmp, mask = RGR1_SW_INIT_1_INIT_GENERIC_MASK; > - u32 shift = RGR1_SW_INIT_1_INIT_GENERIC_SHIFT; > + if (pcie->bridge) { > + if (val) > + reset_control_assert(pcie->bridge); > + else > + reset_control_deassert(pcie->bridge); Please check reset_control_assert/deassert() calls for error. This might need to change the definition of brcm_pcie_bridge_sw_init_set_generic() to return error. ~Stan
On Tue, Jul 2, 2024 at 8:59 AM Stanimir Varbanov <svarbanov@suse.de> wrote: > > > > On 6/28/24 23:54, Jim Quinlan wrote: > > The 7712 SOC has a bridge reset which can be described in the device tree. > > If it is present, use it. Otherwise, continue to use the legacy method to > > reset the bridge. > > > > Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com> > > --- > > drivers/pci/controller/pcie-brcmstb.c | 24 +++++++++++++++++++----- > > 1 file changed, 19 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c > > index c2eb29b886f7..4104c3668fdb 100644 > > --- a/drivers/pci/controller/pcie-brcmstb.c > > +++ b/drivers/pci/controller/pcie-brcmstb.c > > @@ -265,6 +265,7 @@ struct brcm_pcie { > > enum pcie_type type; > > struct reset_control *rescal; > > struct reset_control *perst_reset; > > + struct reset_control *bridge; > > int num_memc; > > u64 memc_size[PCIE_BRCM_MAX_MEMC]; > > u32 hw_rev; > > @@ -732,12 +733,19 @@ static void __iomem *brcm7425_pcie_map_bus(struct pci_bus *bus, > > > > static void brcm_pcie_bridge_sw_init_set_generic(struct brcm_pcie *pcie, u32 val) > > { > > - u32 tmp, mask = RGR1_SW_INIT_1_INIT_GENERIC_MASK; > > - u32 shift = RGR1_SW_INIT_1_INIT_GENERIC_SHIFT; > > + if (pcie->bridge) { > > + if (val) > > + reset_control_assert(pcie->bridge); > > + else > > + reset_control_deassert(pcie->bridge); > > Please check reset_control_assert/deassert() calls for error. This might > need to change the definition of brcm_pcie_bridge_sw_init_set_generic() > to return error. Hi Stan, Do you really think this is necessary? If you look at "drivers/reset/reset-brcmstb.c" there is no way for either of these calls to fail and I don't see that changing because it is just writing a bit into a register. Regards, Jim Quinlan Broadcom STB/CM > > ~Stan
Hi Jim, On 7/2/24 21:36, Jim Quinlan wrote: > On Tue, Jul 2, 2024 at 8:59 AM Stanimir Varbanov <svarbanov@suse.de> wrote: >> >> >> >> On 6/28/24 23:54, Jim Quinlan wrote: >>> The 7712 SOC has a bridge reset which can be described in the device tree. >>> If it is present, use it. Otherwise, continue to use the legacy method to >>> reset the bridge. >>> >>> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com> >>> --- >>> drivers/pci/controller/pcie-brcmstb.c | 24 +++++++++++++++++++----- >>> 1 file changed, 19 insertions(+), 5 deletions(-) >>> >>> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c >>> index c2eb29b886f7..4104c3668fdb 100644 >>> --- a/drivers/pci/controller/pcie-brcmstb.c >>> +++ b/drivers/pci/controller/pcie-brcmstb.c >>> @@ -265,6 +265,7 @@ struct brcm_pcie { >>> enum pcie_type type; >>> struct reset_control *rescal; >>> struct reset_control *perst_reset; >>> + struct reset_control *bridge; >>> int num_memc; >>> u64 memc_size[PCIE_BRCM_MAX_MEMC]; >>> u32 hw_rev; >>> @@ -732,12 +733,19 @@ static void __iomem *brcm7425_pcie_map_bus(struct pci_bus *bus, >>> >>> static void brcm_pcie_bridge_sw_init_set_generic(struct brcm_pcie *pcie, u32 val) >>> { >>> - u32 tmp, mask = RGR1_SW_INIT_1_INIT_GENERIC_MASK; >>> - u32 shift = RGR1_SW_INIT_1_INIT_GENERIC_SHIFT; >>> + if (pcie->bridge) { >>> + if (val) >>> + reset_control_assert(pcie->bridge); >>> + else >>> + reset_control_deassert(pcie->bridge); >> >> Please check reset_control_assert/deassert() calls for error. This might >> need to change the definition of brcm_pcie_bridge_sw_init_set_generic() >> to return error. > > Hi Stan, > > Do you really think this is necessary? If you look at > "drivers/reset/reset-brcmstb.c" there is no way for either of these > calls to fail and I don't see that changing because it is just writing > a bit into a register. yes, I think there are kernel rules which we have to follow. We use generic reset-control interface in pcie driver and we cannot rely on the low-level implementation of this particular reset-controller driver (reset-brcmstb.c). regards, ~Stan
diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c index c2eb29b886f7..4104c3668fdb 100644 --- a/drivers/pci/controller/pcie-brcmstb.c +++ b/drivers/pci/controller/pcie-brcmstb.c @@ -265,6 +265,7 @@ struct brcm_pcie { enum pcie_type type; struct reset_control *rescal; struct reset_control *perst_reset; + struct reset_control *bridge; int num_memc; u64 memc_size[PCIE_BRCM_MAX_MEMC]; u32 hw_rev; @@ -732,12 +733,19 @@ static void __iomem *brcm7425_pcie_map_bus(struct pci_bus *bus, static void brcm_pcie_bridge_sw_init_set_generic(struct brcm_pcie *pcie, u32 val) { - u32 tmp, mask = RGR1_SW_INIT_1_INIT_GENERIC_MASK; - u32 shift = RGR1_SW_INIT_1_INIT_GENERIC_SHIFT; + if (pcie->bridge) { + if (val) + reset_control_assert(pcie->bridge); + else + reset_control_deassert(pcie->bridge); + } else { + u32 tmp, mask = RGR1_SW_INIT_1_INIT_GENERIC_MASK; + u32 shift = RGR1_SW_INIT_1_INIT_GENERIC_SHIFT; - tmp = readl(pcie->base + PCIE_RGR1_SW_INIT_1(pcie)); - tmp = (tmp & ~mask) | ((val << shift) & mask); - writel(tmp, pcie->base + PCIE_RGR1_SW_INIT_1(pcie)); + tmp = readl(pcie->base + PCIE_RGR1_SW_INIT_1(pcie)); + tmp = (tmp & ~mask) | ((val << shift) & mask); + writel(tmp, pcie->base + PCIE_RGR1_SW_INIT_1(pcie)); + } } static void brcm_pcie_bridge_sw_init_set_7278(struct brcm_pcie *pcie, u32 val) @@ -1635,6 +1643,12 @@ static int brcm_pcie_probe(struct platform_device *pdev) goto clk_out; } + pcie->bridge = devm_reset_control_get_optional_exclusive(&pdev->dev, "bridge"); + if (IS_ERR(pcie->bridge)) { + ret = PTR_ERR(pcie->bridge); + goto clk_out; + } + ret = brcm_phy_start(pcie); if (ret) { reset_control_rearm(pcie->rescal);
The 7712 SOC has a bridge reset which can be described in the device tree. If it is present, use it. Otherwise, continue to use the legacy method to reset the bridge. Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com> --- drivers/pci/controller/pcie-brcmstb.c | 24 +++++++++++++++++++----- 1 file changed, 19 insertions(+), 5 deletions(-)