| Message ID | 20240815225731.40276-6-james.quinlan@broadcom.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Krzysztof Wilczyński |
| Headers | show |
| Series | PCI: brcnstb: Enable STB 7712 SOC | expand |
On Thu, Aug 15, 2024 at 06:57:18PM -0400, Jim Quinlan wrote: > The 7712 SOC has a bridge reset which can be described in the device tree. > Use it if present. Otherwise, continue to use the legacy method to reset > the bridge. > > Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> - Mani > --- > 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 790a149f6581..af14debd81d0 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_reset; > 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 (val) > + reset_control_assert(pcie->bridge_reset); > + else > + reset_control_deassert(pcie->bridge_reset); > > - 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)); > + if (!pcie->bridge_reset) { > + 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)); > + } > } > > static void brcm_pcie_bridge_sw_init_set_7278(struct brcm_pcie *pcie, u32 val) > @@ -1621,10 +1629,16 @@ static int brcm_pcie_probe(struct platform_device *pdev) > if (IS_ERR(pcie->perst_reset)) > return PTR_ERR(pcie->perst_reset); > > + pcie->bridge_reset = devm_reset_control_get_optional_exclusive(&pdev->dev, "bridge"); > + if (IS_ERR(pcie->bridge_reset)) > + return PTR_ERR(pcie->bridge_reset); > + > ret = clk_prepare_enable(pcie->clk); > if (ret) > return dev_err_probe(&pdev->dev, ret, "could not enable clock\n"); > > + pcie->bridge_sw_init_set(pcie, 0); > + > ret = reset_control_reset(pcie->rescal); > if (ret) { > clk_disable_unprepare(pcie->clk); > -- > 2.17.1 >
On 8/15/24 15:57, Jim Quinlan wrote: > The 7712 SOC has a bridge reset which can be described in the device tree. > Use it if present. Otherwise, continue to use the legacy method to reset > the bridge. > > Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com> Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
Hi Jim, On 8/16/24 01:57, Jim Quinlan wrote: > The 7712 SOC has a bridge reset which can be described in the device tree. > Use it if present. 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(-) Reviewed-by: Stanimir Varbanov <svarbanov@suse.de> One problem though on RPi5 (bcm2712). With this series applied + my WIP patches for enablement of PCIe on bcm2712 when enable the pcie1 and pcie2 root ports in dts, I see kernel boot stuck on pcie2 enumeration and I have to add this [1] to make it work again. Some more info about resets used: pcie0 @ 100000: resets = <&bcm_reset 5>, <&bcm_reset 42>, <&pcie_rescal>; reset-names = "swinit", "bridge", "rescal"; pcie1 @ 110000: resets = <&bcm_reset 7>, <&bcm_reset 43>, <&pcie_rescal>; reset-names = "swinit", "bridge", "rescal"; pcie2 @ 120000: resets = <&bcm_reset 9>, <&bcm_reset 44>, <&pcie_rescal>; reset-names = "swinit", "bridge", "rescal"; I changed "swinit" reset for pcie2 to <&bcm_reset 9> (it is 32 in downstream rpi kernel) because otherwise I'm unable to enumerate RP1 south bridge at all. Any help will be appreciated. ~Stan [1] https://github.com/raspberrypi/linux/blob/rpi-6.11.y/drivers/pci/controller/pcie-brcmstb.c#L1711
On Sat, Aug 17, 2024 at 1:41 PM Stanimir Varbanov <svarbanov@suse.de> wrote: > > Hi Jim, > > On 8/16/24 01:57, Jim Quinlan wrote: > > The 7712 SOC has a bridge reset which can be described in the device tree. > > Use it if present. 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(-) > > Reviewed-by: Stanimir Varbanov <svarbanov@suse.de> > > One problem though on RPi5 (bcm2712). > > With this series applied + my WIP patches for enablement of PCIe on > bcm2712 when enable the pcie1 and pcie2 root ports in dts, I see kernel > boot stuck on pcie2 enumeration and I have to add this [1] to make it > work again. > > Some more info about resets used: > > pcie0 @ 100000: > resets = <&bcm_reset 5>, <&bcm_reset 42>, <&pcie_rescal>; > reset-names = "swinit", "bridge", "rescal"; > > pcie1 @ 110000: > resets = <&bcm_reset 7>, <&bcm_reset 43>, <&pcie_rescal>; > reset-names = "swinit", "bridge", "rescal"; > > pcie2 @ 120000: > resets = <&bcm_reset 9>, <&bcm_reset 44>, <&pcie_rescal>; > reset-names = "swinit", "bridge", "rescal"; > > > I changed "swinit" reset for pcie2 to <&bcm_reset 9> (it is 32 in > downstream rpi kernel) because otherwise I'm unable to enumerate RP1 > south bridge at all. > > Any help will be appreciated. Hi Stan, Let me look into this. Why is one of the controllers turning off -- is it not populated with a device? As you probably know the 7712 only has access to PCIe1. But we do have another chip with two controllers and I will try to reproduce your failure and get to the bottom of it. Regards, Jim Quinlan Broadcom STB/CM > > ~Stan > > [1] > https://github.com/raspberrypi/linux/blob/rpi-6.11.y/drivers/pci/controller/pcie-brcmstb.c#L1711
On 8/17/24 10:41, Stanimir Varbanov wrote: > Hi Jim, > > On 8/16/24 01:57, Jim Quinlan wrote: >> The 7712 SOC has a bridge reset which can be described in the device tree. >> Use it if present. 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(-) > > Reviewed-by: Stanimir Varbanov <svarbanov@suse.de> > > One problem though on RPi5 (bcm2712). > > With this series applied + my WIP patches for enablement of PCIe on > bcm2712 when enable the pcie1 and pcie2 root ports in dts, I see kernel > boot stuck on pcie2 enumeration and I have to add this [1] to make it > work again. > > Some more info about resets used: > > pcie0 @ 100000: > resets = <&bcm_reset 5>, <&bcm_reset 42>, <&pcie_rescal>; > reset-names = "swinit", "bridge", "rescal"; > > pcie1 @ 110000: > resets = <&bcm_reset 7>, <&bcm_reset 43>, <&pcie_rescal>; > reset-names = "swinit", "bridge", "rescal"; > > pcie2 @ 120000: > resets = <&bcm_reset 9>, <&bcm_reset 44>, <&pcie_rescal>; > reset-names = "swinit", "bridge", "rescal"; > > > I changed "swinit" reset for pcie2 to <&bcm_reset 9> (it is 32 in > downstream rpi kernel) because otherwise I'm unable to enumerate RP1 > south bridge at all. The value 9 is unused, so I suppose it does not really hurt to use it, but it is also unlikely to achieve what you desire. 32 is the correct value since pcie2_sw_init is bit 0 within SW_INIT_1 (second bank of resets). The file link you provided appears to be lacking support for the "swinit" reset line, is that intentional? I don't think you can assume this will work without.
Hi Jim, On 8/19/24 21:09, Jim Quinlan wrote: > On Sat, Aug 17, 2024 at 1:41 PM Stanimir Varbanov <svarbanov@suse.de> wrote: >> >> Hi Jim, >> >> On 8/16/24 01:57, Jim Quinlan wrote: >>> The 7712 SOC has a bridge reset which can be described in the device tree. >>> Use it if present. 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(-) >> >> Reviewed-by: Stanimir Varbanov <svarbanov@suse.de> >> >> One problem though on RPi5 (bcm2712). >> >> With this series applied + my WIP patches for enablement of PCIe on >> bcm2712 when enable the pcie1 and pcie2 root ports in dts, I see kernel >> boot stuck on pcie2 enumeration and I have to add this [1] to make it >> work again. >> >> Some more info about resets used: >> >> pcie0 @ 100000: >> resets = <&bcm_reset 5>, <&bcm_reset 42>, <&pcie_rescal>; >> reset-names = "swinit", "bridge", "rescal"; >> >> pcie1 @ 110000: >> resets = <&bcm_reset 7>, <&bcm_reset 43>, <&pcie_rescal>; >> reset-names = "swinit", "bridge", "rescal"; >> >> pcie2 @ 120000: >> resets = <&bcm_reset 9>, <&bcm_reset 44>, <&pcie_rescal>; >> reset-names = "swinit", "bridge", "rescal"; >> >> >> I changed "swinit" reset for pcie2 to <&bcm_reset 9> (it is 32 in >> downstream rpi kernel) because otherwise I'm unable to enumerate RP1 >> south bridge at all. >> >> Any help will be appreciated. > > Hi Stan, > Let me look into this. Why is one of the controllers turning off -- > is it not populated with a device? Yes, I enabled pcie1 but no PCI endpoint devices attached on the expansion connector. > > As you probably know the 7712 only has access to PCIe1. But we do > have another chip with two controllers and I will try to reproduce > your failure and get to the bottom of it. Thank you for the help. ~Stan > > Regards, > Jim Quinlan > Broadcom STB/CM >> >> ~Stan >> >> [1] >> https://github.com/raspberrypi/linux/blob/rpi-6.11.y/drivers/pci/controller/pcie-brcmstb.c#L1711
On Mon, Aug 19, 2024 at 3:39 PM Stanimir Varbanov <svarbanov@suse.de> wrote: > > Hi Jim, > > On 8/19/24 21:09, Jim Quinlan wrote: > > On Sat, Aug 17, 2024 at 1:41 PM Stanimir Varbanov <svarbanov@suse.de> wrote: > >> > >> Hi Jim, > >> > >> On 8/16/24 01:57, Jim Quinlan wrote: > >>> The 7712 SOC has a bridge reset which can be described in the device tree. > >>> Use it if present. 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(-) > >> > >> Reviewed-by: Stanimir Varbanov <svarbanov@suse.de> > >> > >> One problem though on RPi5 (bcm2712). > >> > >> With this series applied + my WIP patches for enablement of PCIe on > >> bcm2712 when enable the pcie1 and pcie2 root ports in dts, I see kernel > >> boot stuck on pcie2 enumeration and I have to add this [1] to make it > >> work again. > >> > >> Some more info about resets used: > >> > >> pcie0 @ 100000: > >> resets = <&bcm_reset 5>, <&bcm_reset 42>, <&pcie_rescal>; > >> reset-names = "swinit", "bridge", "rescal"; > >> > >> pcie1 @ 110000: > >> resets = <&bcm_reset 7>, <&bcm_reset 43>, <&pcie_rescal>; > >> reset-names = "swinit", "bridge", "rescal"; > >> > >> pcie2 @ 120000: > >> resets = <&bcm_reset 9>, <&bcm_reset 44>, <&pcie_rescal>; > >> reset-names = "swinit", "bridge", "rescal"; > >> > >> > >> I changed "swinit" reset for pcie2 to <&bcm_reset 9> (it is 32 in > >> downstream rpi kernel) because otherwise I'm unable to enumerate RP1 > >> south bridge at all. > >> > >> Any help will be appreciated. > > > > Hi Stan, > > Let me look into this. Why is one of the controllers turning off -- > > is it not populated with a device? > > Yes, I enabled pcie1 but no PCI endpoint devices attached on the > expansion connector. Hi Stan, I looked at our similar STB chip that has a rescal controller and multiple PCIe controllers and it doesn't have this problem. Our 7712 doesn't have this problem either, only because it only has one PCIe controller. However, using my 7712 and unbinding the device (invokes brcm_pcie_remove()) shows me behavior similar to yours (2712). What I do is read the rescal registers after the unbind, and they will either be dead or alive. If I comment out the "pcie->bridge_sw_init_set(pcie, 1);" call, the rescal is still dead after unbind. However if I comment out that AND the "clk_disable_unprepare(pcie->clk);" call, the rescal registers remain alive after unbind. Perhaps you don't see the dependence on the PCIe clocks if the 2712 does not give the PCIe node a clock property and instead keeps its clocks on all of the time. In that case I would think that your solution would be fine. Regards, Jim Quinlan Broadcom STB/CM > > > > > As you probably know the 7712 only has access to PCIe1. But we do > > have another chip with two controllers and I will try to reproduce > > your failure and get to the bottom of it. > > Thank you for the help. > > ~Stan > > > > > Regards, > > Jim Quinlan > > Broadcom STB/CM > >> > >> ~Stan > >> > >> [1] > >> https://github.com/raspberrypi/linux/blob/rpi-6.11.y/drivers/pci/controller/pcie-brcmstb.c#L1711
Hi Florian, On 8/19/24 22:07, Florian Fainelli wrote: > On 8/17/24 10:41, Stanimir Varbanov wrote: >> Hi Jim, >> >> On 8/16/24 01:57, Jim Quinlan wrote: >>> The 7712 SOC has a bridge reset which can be described in the device >>> tree. >>> Use it if present. 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(-) >> >> Reviewed-by: Stanimir Varbanov <svarbanov@suse.de> >> >> One problem though on RPi5 (bcm2712). >> >> With this series applied + my WIP patches for enablement of PCIe on >> bcm2712 when enable the pcie1 and pcie2 root ports in dts, I see kernel >> boot stuck on pcie2 enumeration and I have to add this [1] to make it >> work again. >> >> Some more info about resets used: >> >> pcie0 @ 100000: >> resets = <&bcm_reset 5>, <&bcm_reset 42>, <&pcie_rescal>; >> reset-names = "swinit", "bridge", "rescal"; >> >> pcie1 @ 110000: >> resets = <&bcm_reset 7>, <&bcm_reset 43>, <&pcie_rescal>; >> reset-names = "swinit", "bridge", "rescal"; >> >> pcie2 @ 120000: >> resets = <&bcm_reset 9>, <&bcm_reset 44>, <&pcie_rescal>; >> reset-names = "swinit", "bridge", "rescal"; > >> >> I changed "swinit" reset for pcie2 to <&bcm_reset 9> (it is 32 in >> downstream rpi kernel) because otherwise I'm unable to enumerate RP1 >> south bridge at all. > > The value 9 is unused, so I suppose it does not really hurt to use it, > but it is also unlikely to achieve what you desire. 32 is the correct > value since pcie2_sw_init is bit 0 within SW_INIT_1 (second bank of > resets). Good to know that 9 is not the proper reset line, thank you. Unfortunately, I'm unable to make it work with the proper reset line (32). > > The file link you provided appears to be lacking support for the > "swinit" reset line, is that intentional? I don't think you can assume No idea why downstream RPi kernel does not use swinit reset. > this will work without. If I do not populate swinit in PCIe DT node it works i.e. PCI enumeration is working and RP1 south-bridge is functional. ~Stan
Hi Jim, On 8/20/24 00:49, Jim Quinlan wrote: > On Mon, Aug 19, 2024 at 3:39 PM Stanimir Varbanov <svarbanov@suse.de> wrote: >> >> Hi Jim, >> >> On 8/19/24 21:09, Jim Quinlan wrote: >>> On Sat, Aug 17, 2024 at 1:41 PM Stanimir Varbanov <svarbanov@suse.de> wrote: >>>> >>>> Hi Jim, >>>> >>>> On 8/16/24 01:57, Jim Quinlan wrote: >>>>> The 7712 SOC has a bridge reset which can be described in the device tree. >>>>> Use it if present. 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(-) >>>> >>>> Reviewed-by: Stanimir Varbanov <svarbanov@suse.de> >>>> >>>> One problem though on RPi5 (bcm2712). >>>> >>>> With this series applied + my WIP patches for enablement of PCIe on >>>> bcm2712 when enable the pcie1 and pcie2 root ports in dts, I see kernel >>>> boot stuck on pcie2 enumeration and I have to add this [1] to make it >>>> work again. >>>> >>>> Some more info about resets used: >>>> >>>> pcie0 @ 100000: >>>> resets = <&bcm_reset 5>, <&bcm_reset 42>, <&pcie_rescal>; >>>> reset-names = "swinit", "bridge", "rescal"; >>>> >>>> pcie1 @ 110000: >>>> resets = <&bcm_reset 7>, <&bcm_reset 43>, <&pcie_rescal>; >>>> reset-names = "swinit", "bridge", "rescal"; >>>> >>>> pcie2 @ 120000: >>>> resets = <&bcm_reset 9>, <&bcm_reset 44>, <&pcie_rescal>; >>>> reset-names = "swinit", "bridge", "rescal"; >>>> >>>> >>>> I changed "swinit" reset for pcie2 to <&bcm_reset 9> (it is 32 in >>>> downstream rpi kernel) because otherwise I'm unable to enumerate RP1 >>>> south bridge at all. >>>> >>>> Any help will be appreciated. >>> >>> Hi Stan, >>> Let me look into this. Why is one of the controllers turning off -- >>> is it not populated with a device? >> >> Yes, I enabled pcie1 but no PCI endpoint devices attached on the >> expansion connector. > > Hi Stan, > > I looked at our similar STB chip that has a rescal controller and > multiple PCIe controllers and it doesn't have this problem. Our 7712 > doesn't have this problem either, only because it only has one PCIe > controller. > > However, using my 7712 and unbinding the device (invokes > brcm_pcie_remove()) shows me behavior similar to yours (2712). What I > do is read the rescal registers after the unbind, and they will either > be dead or alive. If I comment out the > "pcie->bridge_sw_init_set(pcie, 1);" call, the rescal is still dead > after unbind. However if I comment out that AND the > "clk_disable_unprepare(pcie->clk);" call, the rescal registers remain > alive after unbind. Thank you. No idea why the clock is not used on 2712 (or at least it is not populated on RPi downstream kernel. > > Perhaps you don't see the dependence on the PCIe clocks if the 2712 > does not give the PCIe node a clock property and instead keeps its > clocks on all of the time. In that case I would think that your > solution would be fine. What you mean by my solution? The one where avoiding assert of bridge_reset at link [1] bellow? If so, I still cannot understand the relation between bridge_reset and rescal as the comment mentions: "Shutting down this bridge on pcie1 means accesses to rescal block will hang the chip if another RC wants to assert/deassert rescal". ~Stan > > Regards, > Jim Quinlan > Broadcom STB/CM > > > >> >>> >>> As you probably know the 7712 only has access to PCIe1. But we do >>> have another chip with two controllers and I will try to reproduce >>> your failure and get to the bottom of it. >> >> Thank you for the help. >> >> ~Stan >> >>> >>> Regards, >>> Jim Quinlan >>> Broadcom STB/CM >>>> >>>> ~Stan >>>> >>>> [1] >>>> https://github.com/raspberrypi/linux/blob/rpi-6.11.y/drivers/pci/controller/pcie-brcmstb.c#L1711
On Tue, Aug 20, 2024 at 7:38 PM Stanimir Varbanov <svarbanov@suse.de> wrote: > > Hi Florian, > > On 8/19/24 22:07, Florian Fainelli wrote: > > On 8/17/24 10:41, Stanimir Varbanov wrote: > >> Hi Jim, > >> > >> On 8/16/24 01:57, Jim Quinlan wrote: > >>> The 7712 SOC has a bridge reset which can be described in the device > >>> tree. > >>> Use it if present. 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(-) > >> > >> Reviewed-by: Stanimir Varbanov <svarbanov@suse.de> > >> > >> One problem though on RPi5 (bcm2712). > >> > >> With this series applied + my WIP patches for enablement of PCIe on > >> bcm2712 when enable the pcie1 and pcie2 root ports in dts, I see kernel > >> boot stuck on pcie2 enumeration and I have to add this [1] to make it > >> work again. > >> > >> Some more info about resets used: > >> > >> pcie0 @ 100000: > >> resets = <&bcm_reset 5>, <&bcm_reset 42>, <&pcie_rescal>; > >> reset-names = "swinit", "bridge", "rescal"; > >> > >> pcie1 @ 110000: > >> resets = <&bcm_reset 7>, <&bcm_reset 43>, <&pcie_rescal>; > >> reset-names = "swinit", "bridge", "rescal"; > >> > >> pcie2 @ 120000: > >> resets = <&bcm_reset 9>, <&bcm_reset 44>, <&pcie_rescal>; > >> reset-names = "swinit", "bridge", "rescal"; > > >> > >> I changed "swinit" reset for pcie2 to <&bcm_reset 9> (it is 32 in > >> downstream rpi kernel) because otherwise I'm unable to enumerate RP1 > >> south bridge at all. > > > > The value 9 is unused, so I suppose it does not really hurt to use it, > > but it is also unlikely to achieve what you desire. 32 is the correct > > value since pcie2_sw_init is bit 0 within SW_INIT_1 (second bank of > > resets). > > Good to know that 9 is not the proper reset line, thank you. > > Unfortunately, I'm unable to make it work with the proper reset line (32). > > > > > The file link you provided appears to be lacking support for the > > "swinit" reset line, is that intentional? I don't think you can assume > > No idea why downstream RPi kernel does not use swinit reset. I found out accidentally that one does not need this (swinit) to properly function. > > > this will work without. > > If I do not populate swinit in PCIe DT node it works i.e. PCI > enumeration is working and RP1 south-bridge is functional. > > ~Stan
On Tue, Aug 20, 2024 at 7:42 PM Stanimir Varbanov <svarbanov@suse.de> wrote: > > Hi Jim, > > On 8/20/24 00:49, Jim Quinlan wrote: > > On Mon, Aug 19, 2024 at 3:39 PM Stanimir Varbanov <svarbanov@suse.de> wrote: > >> > >> Hi Jim, > >> > >> On 8/19/24 21:09, Jim Quinlan wrote: > >>> On Sat, Aug 17, 2024 at 1:41 PM Stanimir Varbanov <svarbanov@suse.de> wrote: > >>>> > >>>> Hi Jim, > >>>> > >>>> On 8/16/24 01:57, Jim Quinlan wrote: > >>>>> The 7712 SOC has a bridge reset which can be described in the device tree. > >>>>> Use it if present. 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(-) > >>>> > >>>> Reviewed-by: Stanimir Varbanov <svarbanov@suse.de> > >>>> > >>>> One problem though on RPi5 (bcm2712). > >>>> > >>>> With this series applied + my WIP patches for enablement of PCIe on > >>>> bcm2712 when enable the pcie1 and pcie2 root ports in dts, I see kernel > >>>> boot stuck on pcie2 enumeration and I have to add this [1] to make it > >>>> work again. > >>>> > >>>> Some more info about resets used: > >>>> > >>>> pcie0 @ 100000: > >>>> resets = <&bcm_reset 5>, <&bcm_reset 42>, <&pcie_rescal>; > >>>> reset-names = "swinit", "bridge", "rescal"; > >>>> > >>>> pcie1 @ 110000: > >>>> resets = <&bcm_reset 7>, <&bcm_reset 43>, <&pcie_rescal>; > >>>> reset-names = "swinit", "bridge", "rescal"; > >>>> > >>>> pcie2 @ 120000: > >>>> resets = <&bcm_reset 9>, <&bcm_reset 44>, <&pcie_rescal>; > >>>> reset-names = "swinit", "bridge", "rescal"; > >>>> > >>>> > >>>> I changed "swinit" reset for pcie2 to <&bcm_reset 9> (it is 32 in > >>>> downstream rpi kernel) because otherwise I'm unable to enumerate RP1 > >>>> south bridge at all. > >>>> > >>>> Any help will be appreciated. > >>> > >>> Hi Stan, > >>> Let me look into this. Why is one of the controllers turning off -- > >>> is it not populated with a device? > >> > >> Yes, I enabled pcie1 but no PCI endpoint devices attached on the > >> expansion connector. > > > > Hi Stan, > > > > I looked at our similar STB chip that has a rescal controller and > > multiple PCIe controllers and it doesn't have this problem. Our 7712 > > doesn't have this problem either, only because it only has one PCIe > > controller. > > > > However, using my 7712 and unbinding the device (invokes > > brcm_pcie_remove()) shows me behavior similar to yours (2712). What I > > do is read the rescal registers after the unbind, and they will either > > be dead or alive. If I comment out the > > "pcie->bridge_sw_init_set(pcie, 1);" call, the rescal is still dead > > after unbind. However if I comment out that AND the > > "clk_disable_unprepare(pcie->clk);" call, the rescal registers remain > > alive after unbind. > > Thank you. No idea why the clock is not used on 2712 (or at least it is > not populated on RPi downstream kernel. Hi Stan, Most of the clocks on the STB chips come up active so one does not have to turn them on and off to have the device function. It helps power savings to do this although I'm not sure it is significant. > > > > > Perhaps you don't see the dependence on the PCIe clocks if the 2712 > > does not give the PCIe node a clock property and instead keeps its > > clocks on all of the time. In that case I would think that your > > solution would be fine. > > What you mean by my solution? The one where avoiding assert of > bridge_reset at link [1] bellow? Yes. > > If so, I still cannot understand the relation between bridge_reset and > rescal as the comment mentions: > > "Shutting down this bridge on pcie1 means accesses to rescal block will > hang the chip if another RC wants to assert/deassert rescal". I was just describing my observations; this should not be happening. I would say it is a HW bug for the 2712. I can file a bug against the 2712 but that will not help us right now. From what I was told by HW, asserting the PCIe1 bridge reset does not affect the rescal settings, but it does freeze access to the rescal registers, and that is game over for the other PCIe controllers accessing the rescal registers. Regards, Jim Quinlan Broadcom STB/CM > > ~Stan > you > > Regards, > > Jim Quinlan > > Broadcom STB/CM > > > > > > > >> > >>> > >>> As you probably know the 7712 only has access to PCIe1. But we do > >>> have another chip with two controllers and I will try to reproduce > >>> your failure and get to the bottom of it. > >> > >> Thank you for the help. > >> > >> ~Stan > >> > >>> > >>> Regards, > >>> Jim Quinlan > >>> Broadcom STB/CM > >>>> > >>>> ~Stan > >>>> > >>>> [1] > >>>> https://github.com/raspberrypi/linux/blob/rpi-6.11.y/drivers/pci/controller/pcie-brcmstb.c#L1711
Hi Jim, <cut> > > Hi Stan, > > Most of the clocks on the STB chips come up active so one does not > have to turn them on and off to have the device function. It helps > power savings to do this although I'm not sure it is significant. >> >>> >>> Perhaps you don't see the dependence on the PCIe clocks if the 2712 >>> does not give the PCIe node a clock property and instead keeps its >>> clocks on all of the time. In that case I would think that your >>> solution would be fine. >> >> What you mean by my solution? The one where avoiding assert of >> bridge_reset at link [1] bellow? > > Yes. >> >> If so, I still cannot understand the relation between bridge_reset and >> rescal as the comment mentions: >> >> "Shutting down this bridge on pcie1 means accesses to rescal block will >> hang the chip if another RC wants to assert/deassert rescal". > > I was just describing my observations; this should not be happening. > I would say it is a HW bug for the 2712. I can file a bug against the > 2712 but that will not help us right now. From what I was told by HW, > asserting the PCIe1 bridge reset does not affect the rescal settings, > but it does freeze access to the rescal registers, and that is game > over for the other PCIe controllers accessing the rescal registers. Good findings, thank you. The problem comes from this snippet from brcm_pcie_probe() : ret = pci_host_probe(bridge); if (!ret && !brcm_pcie_link_up(pcie)) ret = -ENODEV; if (ret) { brcm_pcie_remove(pdev); return ret; } Even when pci_host_probe() is successful the .probe will fail if there are no endpoint devices on this root port bus. This is the case when probing pcie1 port which is the one with external connector. Cause the probe is failing we call reset_control_rearm(rescal) from brcm_pcie_remove(), after that during .probe of pcie2 (the root port where RP1 south-bridge is attached) reset_control_reset(rescal) will issue rescal reset thus rescal-reset driver will stuck on read/write registers. I think we have to drop this link-up check and allow the probe to finish successfully. Even that there no PCI devices attached to bus we want the root port to be visible by lspci tool. This will solve partially the issue with accessing rescal reset-controller registers after asserting bridge_reset. The other part of the problem will be solved by remove the invocation of reset_control_rearm(rescal) from __brcm_pcie_remove(). That way only the first probed root port will issue rescal reset and every next probe will not try to reset rescal because we do not call _rearm(rescal). What do you think? ~Stan
On Mon, Aug 26, 2024 at 6:43 AM Stanimir Varbanov <svarbanov@suse.de> wrote: > > Hi Jim, > > <cut> > > > > > Hi Stan, > > > > Most of the clocks on the STB chips come up active so one does not > > have to turn them on and off to have the device function. It helps > > power savings to do this although I'm not sure it is significant. > >> > >>> > >>> Perhaps you don't see the dependence on the PCIe clocks if the 2712 > >>> does not give the PCIe node a clock property and instead keeps its > >>> clocks on all of the time. In that case I would think that your > >>> solution would be fine. > >> > >> What you mean by my solution? The one where avoiding assert of > >> bridge_reset at link [1] bellow? > > > > Yes. > >> > >> If so, I still cannot understand the relation between bridge_reset and > >> rescal as the comment mentions: > >> > >> "Shutting down this bridge on pcie1 means accesses to rescal block will > >> hang the chip if another RC wants to assert/deassert rescal". > > > > I was just describing my observations; this should not be happening. > > I would say it is a HW bug for the 2712. I can file a bug against the > > 2712 but that will not help us right now. From what I was told by HW, > > asserting the PCIe1 bridge reset does not affect the rescal settings, > > but it does freeze access to the rescal registers, and that is game > > over for the other PCIe controllers accessing the rescal registers. > > Good findings, thank you. > > The problem comes from this snippet from brcm_pcie_probe() : > > ret = pci_host_probe(bridge); > if (!ret && !brcm_pcie_link_up(pcie)) > ret = -ENODEV; > > if (ret) { > brcm_pcie_remove(pdev); > return ret; > } > > Even when pci_host_probe() is successful the .probe will fail if there > are no endpoint devices on this root port bus. This is the case when > probing pcie1 port which is the one with external connector. Cause the > probe is failing we call reset_control_rearm(rescal) from > brcm_pcie_remove(), after that during .probe of pcie2 (the root port > where RP1 south-bridge is attached) reset_control_reset(rescal) will > issue rescal reset thus rescal-reset driver will stuck on read/write > registers. > > I think we have to drop this link-up check and allow the probe to finish > successfully. Even that there no PCI devices attached to bus we want the > root port to be visible by lspci tool. Hi Stan, What is gained by having only the root bridge shown by lspci? We do not support PCIe hotplug so why have lspci reporting a bridge with no devices? The reason we do this is to save power -- when we see no device we turn off the clocks, put things in reset (e.g. bridge), and turn off the regulators. We have SoCs with multiple controllers and they cannot afford to be supplying power to controllers with non-populated sockets; these may be products that are trying to conform to mandated energy-mode specifications. This will solve partially the > issue with accessing rescal reset-controller registers after asserting > bridge_reset. The other part of the problem will be solved by remove the > invocation of reset_control_rearm(rescal) from __brcm_pcie_remove(). > That way only the first probed root port will issue rescal reset and > every next probe will not try to reset rescal because we do not call > _rearm(rescal). In theory I agree with the above -- it should probably not be invoking the rearm() when it is being deactivated. However, I am concerned about a controller(s) going into s2 suspend/resume or unbind/bind. Let me run some tests. Regards, Jim Quinlan Broadcom STB/CM > > What do you think? > > ~Stan
Hi Jim, On 8/26/24 17:17, Jim Quinlan wrote: > On Mon, Aug 26, 2024 at 6:43 AM Stanimir Varbanov <svarbanov@suse.de> wrote: >> >> Hi Jim, >> >> <cut> >> >>> >>> Hi Stan, >>> >>> Most of the clocks on the STB chips come up active so one does not >>> have to turn them on and off to have the device function. It helps >>> power savings to do this although I'm not sure it is significant. >>>> >>>>> >>>>> Perhaps you don't see the dependence on the PCIe clocks if the 2712 >>>>> does not give the PCIe node a clock property and instead keeps its >>>>> clocks on all of the time. In that case I would think that your >>>>> solution would be fine. >>>> >>>> What you mean by my solution? The one where avoiding assert of >>>> bridge_reset at link [1] bellow? >>> >>> Yes. >>>> >>>> If so, I still cannot understand the relation between bridge_reset and >>>> rescal as the comment mentions: >>>> >>>> "Shutting down this bridge on pcie1 means accesses to rescal block will >>>> hang the chip if another RC wants to assert/deassert rescal". >>> >>> I was just describing my observations; this should not be happening. >>> I would say it is a HW bug for the 2712. I can file a bug against the >>> 2712 but that will not help us right now. From what I was told by HW, >>> asserting the PCIe1 bridge reset does not affect the rescal settings, >>> but it does freeze access to the rescal registers, and that is game >>> over for the other PCIe controllers accessing the rescal registers. >> >> Good findings, thank you. >> >> The problem comes from this snippet from brcm_pcie_probe() : >> >> ret = pci_host_probe(bridge); >> if (!ret && !brcm_pcie_link_up(pcie)) >> ret = -ENODEV; >> >> if (ret) { >> brcm_pcie_remove(pdev); >> return ret; >> } >> >> Even when pci_host_probe() is successful the .probe will fail if there >> are no endpoint devices on this root port bus. This is the case when >> probing pcie1 port which is the one with external connector. Cause the >> probe is failing we call reset_control_rearm(rescal) from >> brcm_pcie_remove(), after that during .probe of pcie2 (the root port >> where RP1 south-bridge is attached) reset_control_reset(rescal) will >> issue rescal reset thus rescal-reset driver will stuck on read/write >> registers. >> >> I think we have to drop this link-up check and allow the probe to finish >> successfully. Even that there no PCI devices attached to bus we want the >> root port to be visible by lspci tool. > > Hi Stan, > > What is gained by having only the root bridge shown by lspci? We do > not support PCIe hotplug so why have lspci reporting a bridge with no > devices? > > The reason we do this is to save power -- when we see no device we > turn off the clocks, put things in reset (e.g. bridge), and turn off > the regulators. We have SoCs with multiple controllers and they > cannot afford to be supplying power to controllers with non-populated > sockets; these may be products that are trying to conform to mandated > energy-mode specifications. I totally agree, although I do not see power consumption significantly increased. Also I checked all other PCI controller drivers and no one else doing this. > > This will solve partially the >> issue with accessing rescal reset-controller registers after asserting >> bridge_reset. The other part of the problem will be solved by remove the >> invocation of reset_control_rearm(rescal) from __brcm_pcie_remove(). >> That way only the first probed root port will issue rescal reset and >> every next probe will not try to reset rescal because we do not call >> _rearm(rescal). > > In theory I agree with the above -- it should probably not be invoking > the rearm() when it is being deactivated. However, I am concerned > about a controller(s) going into s2 suspend/resume or unbind/bind. In fact not calling rearm() from __brcm_pcie_remove() is not enough because the .probe will fail thus rescal reset will loose it's instance and next probed pcie root port will issue rescal reset again. I'd stick with avoiding assert of bridge_reset from brcm_pcie_turn_off() for now, and this will be true only for bcm2712. ~Stan
On Tue, Aug 27, 2024 at 8:28 AM Stanimir Varbanov <svarbanov@suse.de> wrote: > > Hi Jim, > > On 8/26/24 17:17, Jim Quinlan wrote: > > On Mon, Aug 26, 2024 at 6:43 AM Stanimir Varbanov <svarbanov@suse.de> wrote: > >> > >> Hi Jim, > >> > >> <cut> > >> > >>> > >>> Hi Stan, > >>> > >>> Most of the clocks on the STB chips come up active so one does not > >>> have to turn them on and off to have the device function. It helps > >>> power savings to do this although I'm not sure it is significant. > >>>> > >>>>> > >>>>> Perhaps you don't see the dependence on the PCIe clocks if the 2712 > >>>>> does not give the PCIe node a clock property and instead keeps its > >>>>> clocks on all of the time. In that case I would think that your > >>>>> solution would be fine. > >>>> > >>>> What you mean by my solution? The one where avoiding assert of > >>>> bridge_reset at link [1] bellow? > >>> > >>> Yes. > >>>> > >>>> If so, I still cannot understand the relation between bridge_reset and > >>>> rescal as the comment mentions: > >>>> > >>>> "Shutting down this bridge on pcie1 means accesses to rescal block will > >>>> hang the chip if another RC wants to assert/deassert rescal". > >>> > >>> I was just describing my observations; this should not be happening. > >>> I would say it is a HW bug for the 2712. I can file a bug against the > >>> 2712 but that will not help us right now. From what I was told by HW, > >>> asserting the PCIe1 bridge reset does not affect the rescal settings, > >>> but it does freeze access to the rescal registers, and that is game > >>> over for the other PCIe controllers accessing the rescal registers. > >> > >> Good findings, thank you. > >> > >> The problem comes from this snippet from brcm_pcie_probe() : > >> > >> ret = pci_host_probe(bridge); > >> if (!ret && !brcm_pcie_link_up(pcie)) > >> ret = -ENODEV; > >> > >> if (ret) { > >> brcm_pcie_remove(pdev); > >> return ret; > >> } > >> > >> Even when pci_host_probe() is successful the .probe will fail if there > >> are no endpoint devices on this root port bus. This is the case when > >> probing pcie1 port which is the one with external connector. Cause the > >> probe is failing we call reset_control_rearm(rescal) from > >> brcm_pcie_remove(), after that during .probe of pcie2 (the root port > >> where RP1 south-bridge is attached) reset_control_reset(rescal) will > >> issue rescal reset thus rescal-reset driver will stuck on read/write > >> registers. > >> > >> I think we have to drop this link-up check and allow the probe to finish > >> successfully. Even that there no PCI devices attached to bus we want the > >> root port to be visible by lspci tool. > > > > Hi Stan, > > > > What is gained by having only the root bridge shown by lspci? We do > > not support PCIe hotplug so why have lspci reporting a bridge with no > > devices? > > > > The reason we do this is to save power -- when we see no device we > > turn off the clocks, put things in reset (e.g. bridge), and turn off > > the regulators. We have SoCs with multiple controllers and they > > cannot afford to be supplying power to controllers with non-populated > > sockets; these may be products that are trying to conform to mandated > > energy-mode specifications. > > I totally agree, although I do not see power consumption significantly > increased. Also I checked all other PCI controller drivers and no one > else doing this. Hi Stan, I see a few drivers use runtime_pm but we do not; our architecture is old and not amenable to that system. Discounting runtime_pm, we seem to be the only controller that uses the .suspend* or .resume* methods. I think we are kind of unique in how we turn off/on voltage regulators for the endpoint device -- we put the regulator node under the port driver DT node. This is what customers asked for and what made customers happy -- they just add a regulator node and ref to the DT we provide and they don't have to worry about syncing power for their device on their board. There was also a recent patch submission that implemented something similar by making the port driver customizable so that it could pick up regulators; I don't recall if that ever got accepted. But if it was, we were thinking of switching to it. Let me ask the HW folks if it is acceptable to leave the bridge reset unasserted on remove() or suspend(). Note that if RPi decides to give <clocks> and <clock-names> valid props then problem will still be there. Regards, Jim Quiinlan Broad > > > > > This will solve partially the > >> issue with accessing rescal reset-controller registers after asserting > >> bridge_reset. The other part of the problem will be solved by remove the > >> invocation of reset_control_rearm(rescal) from __brcm_pcie_remove(). > >> That way only the first probed root port will issue rescal reset and > >> every next probe will not try to reset rescal because we do not call > >> _rearm(rescal). > > > > In theory I agree with the above -- it should probably not be invoking > > the rearm() when it is being deactivated. However, I am concerned > > about a controller(s) going into s2 suspend/resume or unbind/bind. > > In fact not calling rearm() from __brcm_pcie_remove() is not enough > because the .probe will fail thus rescal reset will loose it's instance > and next probed pcie root port will issue rescal reset again. > > I'd stick with avoiding assert of bridge_reset from brcm_pcie_turn_off() > for now, and this will be true only for bcm2712. > > ~Stan
Hello, [...] > Let me ask the HW folks if it is acceptable to leave the bridge reset > unasserted on remove() or suspend(). Note that if RPi decides to give > <clocks> and <clock-names> valid props then problem will still be > there. I took this series, but if you have some concerns, especially given the conversation here, then do let me know... We can always hold for now, and get whatever changes you would need to be part of v7. Krzysztof
On Thu, Aug 15, 2024 at 06:57:18PM -0400, Jim Quinlan wrote: > The 7712 SOC has a bridge reset which can be described in the device tree. > Use it if present. Otherwise, continue to use the legacy method to reset > the bridge. > 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 (val) > + reset_control_assert(pcie->bridge_reset); > + else > + reset_control_deassert(pcie->bridge_reset); > > - 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)); > + if (!pcie->bridge_reset) { > + 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)); > + } This pattern looks goofy: reset_control_assert(pcie->bridge_reset); if (!pcie->bridge_reset) { ... If we're going to test pcie->bridge_reset at all, it should be first so it's obvious what's going on and the reader doesn't have to go verify that reset_control_assert() ignores and returns success for a NULL pointer: if (pcie->bridge_reset) { if (val) reset_control_assert(pcie->bridge_reset); else reset_control_deassert(pcie->bridge_reset); return; } u32 tmp, mask = RGR1_SW_INIT_1_INIT_GENERIC_MASK; ... Krzysztof, can you amend this on the branch? It will also make the eventual return checking and error message simpler because we won't have to initialize "ret" first, and we can "return 0" directly for the legacy case. Bjorn
On Mon, Sep 2, 2024 at 3:18 PM Bjorn Helgaas <helgaas@kernel.org> wrote: > > On Thu, Aug 15, 2024 at 06:57:18PM -0400, Jim Quinlan wrote: > > The 7712 SOC has a bridge reset which can be described in the device tree. > > Use it if present. Otherwise, continue to use the legacy method to reset > > the bridge. > > > 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 (val) > > + reset_control_assert(pcie->bridge_reset); > > + else > > + reset_control_deassert(pcie->bridge_reset); > > > > - 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)); > > + if (!pcie->bridge_reset) { > > + 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)); > > + } > > This pattern looks goofy: > > reset_control_assert(pcie->bridge_reset); > if (!pcie->bridge_reset) { > ... > > If we're going to test pcie->bridge_reset at all, it should be first > so it's obvious what's going on and the reader doesn't have to go > verify that reset_control_assert() ignores and returns success for a > NULL pointer: > > if (pcie->bridge_reset) { > if (val) > reset_control_assert(pcie->bridge_reset); > else > reset_control_deassert(pcie->bridge_reset); > > return; > } > > u32 tmp, mask = RGR1_SW_INIT_1_INIT_GENERIC_MASK; > ... > Will do. Jim Quinlan Broadcom STB/CM > Krzysztof, can you amend this on the branch? > > It will also make the eventual return checking and error message > simpler because we won't have to initialize "ret" first, and we can > "return 0" directly for the legacy case. > > Bjorn
Hello, [...] > > > 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 (val) > > > + reset_control_assert(pcie->bridge_reset); > > > + else > > > + reset_control_deassert(pcie->bridge_reset); > > > > > > - 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)); > > > + if (!pcie->bridge_reset) { > > > + 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)); > > > + } > > > > This pattern looks goofy: > > > > reset_control_assert(pcie->bridge_reset); > > if (!pcie->bridge_reset) { > > ... > > > > If we're going to test pcie->bridge_reset at all, it should be first > > so it's obvious what's going on and the reader doesn't have to go > > verify that reset_control_assert() ignores and returns success for a > > NULL pointer: > > > > if (pcie->bridge_reset) { > > if (val) > > reset_control_assert(pcie->bridge_reset); > > else > > reset_control_deassert(pcie->bridge_reset); > > > > return; > > } > > > > u32 tmp, mask = RGR1_SW_INIT_1_INIT_GENERIC_MASK; > > ... > > > Will do. [...] You will do what? If you don't mind me asking. Krzysztof
On Tue, Sep 03, 2024 at 11:46:13PM +0900, Krzysztof Wilczyński wrote: > Hello, > > [...] > > > > 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 (val) > > > > + reset_control_assert(pcie->bridge_reset); > > > > + else > > > > + reset_control_deassert(pcie->bridge_reset); > > > > > > > > - 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)); > > > > + if (!pcie->bridge_reset) { > > > > + 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)); > > > > + } > > > > > > This pattern looks goofy: > > > > > > reset_control_assert(pcie->bridge_reset); > > > if (!pcie->bridge_reset) { > > > ... > > > > > > If we're going to test pcie->bridge_reset at all, it should be first > > > so it's obvious what's going on and the reader doesn't have to go > > > verify that reset_control_assert() ignores and returns success for a > > > NULL pointer: > > > > > > if (pcie->bridge_reset) { > > > if (val) > > > reset_control_assert(pcie->bridge_reset); > > > else > > > reset_control_deassert(pcie->bridge_reset); > > > > > > return; > > > } > > > > > > u32 tmp, mask = RGR1_SW_INIT_1_INIT_GENERIC_MASK; > > > ... > > > > > Will do. > [...] > > You will do what? If you don't mind me asking. Can you just do the rework on the branch, Krzysztof? I think that will be easier/quicker than having Jim repost the entire series. Bjorn
Hello, [...] > > > > > 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 (val) > > > > > + reset_control_assert(pcie->bridge_reset); > > > > > + else > > > > > + reset_control_deassert(pcie->bridge_reset); > > > > > > > > > > - 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)); > > > > > + if (!pcie->bridge_reset) { > > > > > + 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)); > > > > > + } > > > > > > > > This pattern looks goofy: > > > > > > > > reset_control_assert(pcie->bridge_reset); > > > > if (!pcie->bridge_reset) { > > > > ... > > > > > > > > If we're going to test pcie->bridge_reset at all, it should be first > > > > so it's obvious what's going on and the reader doesn't have to go > > > > verify that reset_control_assert() ignores and returns success for a > > > > NULL pointer: > > > > > > > > if (pcie->bridge_reset) { > > > > if (val) > > > > reset_control_assert(pcie->bridge_reset); > > > > else > > > > reset_control_deassert(pcie->bridge_reset); > > > > > > > > return; > > > > } > > > > > > > > u32 tmp, mask = RGR1_SW_INIT_1_INIT_GENERIC_MASK; > > > > ... > > > > > > > Will do. > > [...] > > > > You will do what? If you don't mind me asking. > > Can you just do the rework on the branch, Krzysztof? I think that > will be easier/quicker than having Jim repost the entire series. Will do. That was the idea, I believe. Krzysztof
On Tue, Sep 3, 2024 at 10:26 AM Jim Quinlan <james.quinlan@broadcom.com> wrote: > > On Mon, Sep 2, 2024 at 3:18 PM Bjorn Helgaas <helgaas@kernel.org> wrote: > > > > On Thu, Aug 15, 2024 at 06:57:18PM -0400, Jim Quinlan wrote: > > > The 7712 SOC has a bridge reset which can be described in the device tree. > > > Use it if present. Otherwise, continue to use the legacy method to reset > > > the bridge. > > > > > 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 (val) > > > + reset_control_assert(pcie->bridge_reset); > > > + else > > > + reset_control_deassert(pcie->bridge_reset); > > > > > > - 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)); > > > + if (!pcie->bridge_reset) { > > > + 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)); > > > + } > > > > This pattern looks goofy: > > > > reset_control_assert(pcie->bridge_reset); > > if (!pcie->bridge_reset) { > > ... > > > > If we're going to test pcie->bridge_reset at all, it should be first > > so it's obvious what's going on and the reader doesn't have to go > > verify that reset_control_assert() ignores and returns success for a > > NULL pointer: > > > > if (pcie->bridge_reset) { > > if (val) > > reset_control_assert(pcie->bridge_reset); > > else > > reset_control_deassert(pcie->bridge_reset); > > > > return; > > } > > > > u32 tmp, mask = RGR1_SW_INIT_1_INIT_GENERIC_MASK; > > ... > > > Will do. Hi Bjorn, It is not clear to me if you want a new series -- which would be V7 -- or you are okay with the current series V6. If the latter, someone sent in a fixup commit which must be included. Please advise. Jim Quinlan Broadcom STB/CM > > > Jim Quinlan > Broadcom STB/CM > > > Krzysztof, can you amend this on the branch? > > > > It will also make the eventual return checking and error message > > simpler because we won't have to initialize "ret" first, and we can > > "return 0" directly for the legacy case. > > > > Bjorn
On Tue, Sep 10, 2024 at 01:30:41PM -0400, Jim Quinlan wrote: > On Tue, Sep 3, 2024 at 10:26 AM Jim Quinlan <james.quinlan@broadcom.com> wrote: > > On Mon, Sep 2, 2024 at 3:18 PM Bjorn Helgaas <helgaas@kernel.org> wrote: > > > On Thu, Aug 15, 2024 at 06:57:18PM -0400, Jim Quinlan wrote: > > > > The 7712 SOC has a bridge reset which can be described in the device tree. > > > > Use it if present. Otherwise, continue to use the legacy method to reset > > > > the bridge. > > > > > > > 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 (val) > > > > + reset_control_assert(pcie->bridge_reset); > > > > + else > > > > + reset_control_deassert(pcie->bridge_reset); > > > > > > > > - 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)); > > > > + if (!pcie->bridge_reset) { > > > > + 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)); > > > > + } > > > > > > This pattern looks goofy: > > > > > > reset_control_assert(pcie->bridge_reset); > > > if (!pcie->bridge_reset) { > > > ... > > > > > > If we're going to test pcie->bridge_reset at all, it should be first > > > so it's obvious what's going on and the reader doesn't have to go > > > verify that reset_control_assert() ignores and returns success for a > > > NULL pointer: > > > > > > if (pcie->bridge_reset) { > > > if (val) > > > reset_control_assert(pcie->bridge_reset); > > > else > > > reset_control_deassert(pcie->bridge_reset); > > > > > > return; > > > } > > > > > > u32 tmp, mask = RGR1_SW_INIT_1_INIT_GENERIC_MASK; > > > ... > > > > > Will do. > > Hi Bjorn, > > It is not clear to me if you want a new series -- which would be V7 -- > or you are okay with the current series V6. If the latter, someone > sent in a fixup commit which must be included. > Please advise. Krzysztof amended this on the branch. Take a look here and verify that it makes sense to you: https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/tree/drivers/pci/controller/pcie-brcmstb.c?h=controller/brcmstb#n752 If that looks right to you, no need to post a new v7. I think Krzysztof also integrated an "int num_inbound_wins" fix; is that the one you mean? If I'm thinking of the right one, you can check that at: https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/tree/drivers/pci/controller/pcie-brcmstb.c?h=controller/brcmstb#n1034 > > > Krzysztof, can you amend this on the branch? > > > > > > It will also make the eventual return checking and error message > > > simpler because we won't have to initialize "ret" first, and we can > > > "return 0" directly for the legacy case. > > > > > > Bjorn
Hello, [...] > > > Will do. > > > > It is not clear to me if you want a new series -- which would be V7 -- > > or you are okay with the current series V6. If the latter, someone > > sent in a fixup commit which must be included. > > Please advise. Apologies for the confusion. I though it was obvious that I will go ahead and fix the code on the branch directly. > Krzysztof amended this on the branch. Take a look here and verify > that it makes sense to you: > > https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/tree/drivers/pci/controller/pcie-brcmstb.c?h=controller/brcmstb#n752 > > If that looks right to you, no need to post a new v7. > > I think Krzysztof also integrated an "int num_inbound_wins" fix; is > that the one you mean? If I'm thinking of the right one, you can > check that at: > > https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/tree/drivers/pci/controller/pcie-brcmstb.c?h=controller/brcmstb#n1034 Let me know if anything else needs doing. But, if anything, then we need to be quick about it. Krzysztof
On Tue, Sep 10, 2024 at 1:59 PM Bjorn Helgaas <helgaas@kernel.org> wrote: > > On Tue, Sep 10, 2024 at 01:30:41PM -0400, Jim Quinlan wrote: > > On Tue, Sep 3, 2024 at 10:26 AM Jim Quinlan <james.quinlan@broadcom.com> wrote: > > > On Mon, Sep 2, 2024 at 3:18 PM Bjorn Helgaas <helgaas@kernel.org> wrote: > > > > On Thu, Aug 15, 2024 at 06:57:18PM -0400, Jim Quinlan wrote: > > > > > The 7712 SOC has a bridge reset which can be described in the device tree. > > > > > Use it if present. Otherwise, continue to use the legacy method to reset > > > > > the bridge. > > > > > > > > > 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 (val) > > > > > + reset_control_assert(pcie->bridge_reset); > > > > > + else > > > > > + reset_control_deassert(pcie->bridge_reset); > > > > > > > > > > - 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)); > > > > > + if (!pcie->bridge_reset) { > > > > > + 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)); > > > > > + } > > > > > > > > This pattern looks goofy: > > > > > > > > reset_control_assert(pcie->bridge_reset); > > > > if (!pcie->bridge_reset) { > > > > ... > > > > > > > > If we're going to test pcie->bridge_reset at all, it should be first > > > > so it's obvious what's going on and the reader doesn't have to go > > > > verify that reset_control_assert() ignores and returns success for a > > > > NULL pointer: > > > > > > > > if (pcie->bridge_reset) { > > > > if (val) > > > > reset_control_assert(pcie->bridge_reset); > > > > else > > > > reset_control_deassert(pcie->bridge_reset); > > > > > > > > return; > > > > } > > > > > > > > u32 tmp, mask = RGR1_SW_INIT_1_INIT_GENERIC_MASK; > > > > ... > > > > > > > Will do. > > > > Hi Bjorn, > > > > It is not clear to me if you want a new series -- which would be V7 -- > > or you are okay with the current series V6. If the latter, someone > > sent in a fixup commit which must be included. > > Please advise. > > Krzysztof amended this on the branch. Take a look here and verify > that it makes sense to you: > > https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/tree/drivers/pci/controller/pcie-brcmstb.c?h=controller/brcmstb#n752 > > If that looks right to you, no need to post a new v7. > > I think Krzysztof also integrated an "int num_inbound_wins" fix; is > that the one you mean? If I'm thinking of the right one, you can > check that at: > > https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/tree/drivers/pci/controller/pcie-brcmstb.c?h=controller/brcmstb#n1034 > > > > > Krzysztof, can you amend this on the branch? > > > > > > > > It will also make the eventual return checking and error message > > > > simpler because we won't have to initialize "ret" first, and we can > > > > "return 0" directly for the legacy case. > > > > > > > > Bjorn Sorry, I didn't see this email until now. The changes look good, thanks for making them. Regards, Jim > >
diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c index 790a149f6581..af14debd81d0 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_reset; 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 (val) + reset_control_assert(pcie->bridge_reset); + else + reset_control_deassert(pcie->bridge_reset); - 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)); + if (!pcie->bridge_reset) { + 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)); + } } static void brcm_pcie_bridge_sw_init_set_7278(struct brcm_pcie *pcie, u32 val) @@ -1621,10 +1629,16 @@ static int brcm_pcie_probe(struct platform_device *pdev) if (IS_ERR(pcie->perst_reset)) return PTR_ERR(pcie->perst_reset); + pcie->bridge_reset = devm_reset_control_get_optional_exclusive(&pdev->dev, "bridge"); + if (IS_ERR(pcie->bridge_reset)) + return PTR_ERR(pcie->bridge_reset); + ret = clk_prepare_enable(pcie->clk); if (ret) return dev_err_probe(&pdev->dev, ret, "could not enable clock\n"); + pcie->bridge_sw_init_set(pcie, 0); + ret = reset_control_reset(pcie->rescal); if (ret) { clk_disable_unprepare(pcie->clk);
The 7712 SOC has a bridge reset which can be described in the device tree. Use it if present. 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(-)