| Message ID | 20240731222831.14895-5-james.quinlan@broadcom.com |
|---|---|
| State | New |
| Headers | show |
| Series | PCI: brcnstb: Enable STB 7712 SOC | expand |
On 7/31/24 15:28, 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>
On Wed, Jul 31, 2024 at 06:28: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 7595e7009192..4d68fe318178 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 (dev_err_probe(&pdev->dev, ret, "failed to deassert 'rescal'\n")) > goto clk_disable_unprepare; > -- > 2.17.1 >
Hi Jim, On 8/1/24 01:28, 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(-) > > diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c > index 7595e7009192..4d68fe318178 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"); Shouldn't this be devm_reset_control_get_optional_shared? See more below. > + 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); According to reset_control_get_shared description looks like this .deassert is satisfying the requirements for _shared reset-control API variant. Is that the intention to call reset_control_deassert() here? > + > ret = reset_control_reset(pcie->rescal); > if (dev_err_probe(&pdev->dev, ret, "failed to deassert 'rescal'\n")) > goto clk_disable_unprepare; ~Stan
On Fri, Aug 9, 2024 at 7:16 AM Stanimir Varbanov <svarbanov@suse.de> wrote: > > Hi Jim, > > On 8/1/24 01:28, 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(-) > > > > diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c > > index 7595e7009192..4d68fe318178 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"); > > Shouldn't this be devm_reset_control_get_optional_shared? See more below. Hi Stan, Yes you are correct, thank you. It is shared with the driver in drivers/ata/ahci_brcm.c. I don't know how this got changed to exclusive. Will fix. Regards, Jim Quinlan Broadcom STB/CM > > > + 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); > > According to reset_control_get_shared description looks like this > .deassert is satisfying the requirements for _shared reset-control API > variant. > Is that the intention to call reset_control_deassert() here? > > > + > > ret = reset_control_reset(pcie->rescal); > > if (dev_err_probe(&pdev->dev, ret, "failed to deassert 'rescal'\n")) > > goto clk_disable_unprepare; > > ~Stan
On Fri, Aug 9, 2024 at 7:16 AM Stanimir Varbanov <svarbanov@suse.de> wrote: > > Hi Jim, > > On 8/1/24 01:28, 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(-) > > > > diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c > > index 7595e7009192..4d68fe318178 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"); > > Shouldn't this be devm_reset_control_get_optional_shared? See more below. > > > + 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); > > According to reset_control_get_shared description looks like this > .deassert is satisfying the requirements for _shared reset-control API > variant. > Is that the intention to call reset_control_deassert() here? Hi Stan, Now I'm not sure I understand you. All of the resets (bridge, perst, swinit) are exclusive, except for the "rescal" reset, which is shared. On the exclusive resets I am using reset_control_assert() and reset_control_deasssert(). On the shared reset I am using reset_control_rearm() and reset_control_reset(). Why do you think the calls I am using are incongruent with the shard/exclusive status? Regards, Jim Quinlan Broadcom STB/CM > > > + > > ret = reset_control_reset(pcie->rescal); > > if (dev_err_probe(&pdev->dev, ret, "failed to deassert 'rescal'\n")) > > goto clk_disable_unprepare; > > ~Stan
Hi Jim, On 8/12/24 18:46, Jim Quinlan wrote: > On Fri, Aug 9, 2024 at 7:16 AM Stanimir Varbanov <svarbanov@suse.de> wrote: >> >> Hi Jim, >> >> On 8/1/24 01:28, 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(-) >>> >>> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c >>> index 7595e7009192..4d68fe318178 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"); >> >> Shouldn't this be devm_reset_control_get_optional_shared? See more below. >> >>> + 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); ^^^ this was missing in v4 >> >> According to reset_control_get_shared description looks like this >> .deassert is satisfying the requirements for _shared reset-control API >> variant. >> Is that the intention to call reset_control_deassert() here? > > Hi Stan, > Now I'm not sure I understand you. All of the resets (bridge, perst, > swinit) are exclusive, except for the "rescal" reset, which is shared. ... the call of pcie->bridge_sw_init_set(pcie, 0) from brcm_pcie_probe() was missing in previous v4 and I'm wondering what changed in v5. And because of _shared reset-control description [1] I decided (wrongly) that the bridge reset could be _shared_. > > On the exclusive resets I am using reset_control_assert() and > reset_control_deasssert(). On the shared reset I am using > reset_control_rearm() and reset_control_reset(). Why do > you think the calls I am using are incongruent with the shard/exclusive > status? I'd argue that rescal reset is not correctly used in brcm_pcie_probe(), see [2] and according to [1] "Calling reset_control_reset is also not allowed on a shared reset control." [1] https://elixir.bootlin.com/linux/v6.11-rc3/source/include/linux/reset.h#L338 [2] https://elixir.bootlin.com/linux/v6.11-rc3/source/drivers/pci/controller/pcie-brcmstb.c#L1632 ~Stan
On 8/12/24 18:28, Stanimir Varbanov wrote: > Hi Jim, > > On 8/12/24 18:46, Jim Quinlan wrote: >> On Fri, Aug 9, 2024 at 7:16 AM Stanimir Varbanov <svarbanov@suse.de> wrote: >>> Hi Jim, >>> >>> On 8/1/24 01:28, 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(-) >>>> >>>> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c >>>> index 7595e7009192..4d68fe318178 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"); >>> Shouldn't this be devm_reset_control_get_optional_shared? See more below. >>> >>>> + 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); > ^^^ this was missing in v4 Hi Stan, You are correct and I was remiss in not mentioning this in the cover letter. After my changes on V4 I discovered that my driver failed the rebind in my unbind/rebind test and this line was required. > >>> According to reset_control_get_shared description looks like this >>> .deassert is satisfying the requirements for _shared reset-control API >>> variant. >>> Is that the intention to call reset_control_deassert() here? >> Hi Stan, >> Now I'm not sure I understand you. All of the resets (bridge, perst, >> swinit) are exclusive, except for the "rescal" reset, which is shared. > ... the call of pcie->bridge_sw_init_set(pcie, 0) from brcm_pcie_probe() > was missing in previous v4 and I'm wondering what changed in v5. > > And because of _shared reset-control description [1] I decided (wrongly) > that the bridge reset could be _shared_. > >> On the exclusive resets I am using reset_control_assert() and >> reset_control_deasssert(). On the shared reset I am using >> reset_control_rearm() and reset_control_reset(). Why do >> you think the calls I am using are incongruent with the shard/exclusive >> status? > I'd argue that rescal reset is not correctly used in brcm_pcie_probe(), > see [2] and according to [1] "Calling reset_control_reset is also not > allowed on a shared reset control." This is interesting because in reset/core.c [1] the comment says that "reset_control_rearm", which is clearly used for shared resets, must be paired with calls to "reset_control_reset". Regards, Jim Quinlan Broadcom STB/CM [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/reset/core.c?h=v6.11-rc3#n412 > > > [1] > https://elixir.bootlin.com/linux/v6.11-rc3/source/include/linux/reset.h#L338 > > [2] > https://elixir.bootlin.com/linux/v6.11-rc3/source/drivers/pci/controller/pcie-brcmstb.c#L1632 > > ~Stan
diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c index 7595e7009192..4d68fe318178 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 (dev_err_probe(&pdev->dev, ret, "failed to deassert 'rescal'\n")) goto clk_disable_unprepare;
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(-)