Re: [PATCH 07/15] PCI: brcmstb: Add control of rescal reset
On 5/21/2020 2:48 PM, Jim Quinlan wrote: > On Wed, May 20, 2020 at 3:27 AM Philipp Zabel wrote: >> >> Hi Jim, >> >> On Tue, May 19, 2020 at 04:34:05PM -0400, Jim Quinlan wrote: >>> From: Jim Quinlan >>> >>> Some STB chips have a special purpose reset controller named >>> RESCAL (reset calibration). This commit adds the control >>> of RESCAL as well as the ability to start and stop its >>> operation for PCIe HW. >>> >>> Signed-off-by: Jim Quinlan >>> --- >>> drivers/pci/controller/pcie-brcmstb.c | 81 ++- >>> 1 file changed, 80 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/pci/controller/pcie-brcmstb.c >>> b/drivers/pci/controller/pcie-brcmstb.c >>> index 2c470104ba38..0787e8f6f7e5 100644 >>> --- a/drivers/pci/controller/pcie-brcmstb.c >>> +++ b/drivers/pci/controller/pcie-brcmstb.c >> [...] >>> @@ -1100,6 +1164,21 @@ static int brcm_pcie_probe(struct platform_device >>> *pdev) >>> dev_err(>dev, "could not enable clock\n"); >>> return ret; >>> } >>> + pcie->rescal = devm_reset_control_get_shared(>dev, "rescal"); >>> + if (IS_ERR(pcie->rescal)) { >>> + if (PTR_ERR(pcie->rescal) == -EPROBE_DEFER) >>> + return -EPROBE_DEFER; >>> + pcie->rescal = NULL; >> >> This is effectively an optional reset control, so it is better to use: >> ↵ >> pcie->rescal = devm_reset_control_get_optional_shared(>dev, >> "rescal");↵ >> if (IS_ERR(pcie->rescal)) >> return PTR_ERR(pcie->rescal); >> >>> + } else { >>> + ret = reset_control_deassert(pcie->rescal); >>> + if (ret) >>> + dev_err(>dev, "failed to deassert 'rescal'\n"); >>> + } >> >> reset_control_* can handle rstc == NULL parameters for optional reset >> controls, so this can be done unconditionally: >> >> ret = reset_control_deassert(pcie->rescal);↵ >> if (ret)↵ >> dev_err(>dev, "failed to deassert 'rescal'\n");↵ >> >> Is rescal handled by the reset-brcmstb-rescal driver? Since that only >> implements the .reset op, I would expect reset_control_reset() here. > Where exactly? "reset.h" says that "Calling reset_control_rese()t is > not allowed on a shared reset control." so I'm not sure why you would > want me to invoke it. Yes this is handled by drivers/reset/reset-brcmstb-rescal.c which only implements a .reset() callback, what would be the appropriate API usage here given that this is a shared reset between AHCI and PCIe, should drivers/reset/reset-brcmstb-rescal.c not implement a .reset() callback and .assert() callback instead? >> Otherwise this looks like it'd be missing a reset_control_assert in >> remove. > I can add this. > Thanks, > Jim >> >> regards >> Philipp -- Florian
Re: [PATCH 07/15] PCI: brcmstb: Add control of rescal reset
On Wed, May 20, 2020 at 3:27 AM Philipp Zabel wrote: > > Hi Jim, > > On Tue, May 19, 2020 at 04:34:05PM -0400, Jim Quinlan wrote: > > From: Jim Quinlan > > > > Some STB chips have a special purpose reset controller named > > RESCAL (reset calibration). This commit adds the control > > of RESCAL as well as the ability to start and stop its > > operation for PCIe HW. > > > > Signed-off-by: Jim Quinlan > > --- > > drivers/pci/controller/pcie-brcmstb.c | 81 ++- > > 1 file changed, 80 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/pci/controller/pcie-brcmstb.c > > b/drivers/pci/controller/pcie-brcmstb.c > > index 2c470104ba38..0787e8f6f7e5 100644 > > --- a/drivers/pci/controller/pcie-brcmstb.c > > +++ b/drivers/pci/controller/pcie-brcmstb.c > [...] > > @@ -1100,6 +1164,21 @@ static int brcm_pcie_probe(struct platform_device > > *pdev) > > dev_err(>dev, "could not enable clock\n"); > > return ret; > > } > > + pcie->rescal = devm_reset_control_get_shared(>dev, "rescal"); > > + if (IS_ERR(pcie->rescal)) { > > + if (PTR_ERR(pcie->rescal) == -EPROBE_DEFER) > > + return -EPROBE_DEFER; > > + pcie->rescal = NULL; > > This is effectively an optional reset control, so it is better to use: > ↵ > pcie->rescal = devm_reset_control_get_optional_shared(>dev, > "rescal");↵ > if (IS_ERR(pcie->rescal)) > return PTR_ERR(pcie->rescal); > > > + } else { > > + ret = reset_control_deassert(pcie->rescal); > > + if (ret) > > + dev_err(>dev, "failed to deassert 'rescal'\n"); > > + } > > reset_control_* can handle rstc == NULL parameters for optional reset > controls, so this can be done unconditionally: > > ret = reset_control_deassert(pcie->rescal);↵ > if (ret)↵ > dev_err(>dev, "failed to deassert 'rescal'\n");↵ > > Is rescal handled by the reset-brcmstb-rescal driver? Since that only > implements the .reset op, I would expect reset_control_reset() here. Where exactly? "reset.h" says that "Calling reset_control_rese()t is not allowed on a shared reset control." so I'm not sure why you would want me to invoke it. > Otherwise this looks like it'd be missing a reset_control_assert in > remove. I can add this. Thanks, Jim > > regards > Philipp
Re: [PATCH 07/15] PCI: brcmstb: Add control of rescal reset
Hi Jim, On Tue, May 19, 2020 at 04:34:05PM -0400, Jim Quinlan wrote: > From: Jim Quinlan > > Some STB chips have a special purpose reset controller named > RESCAL (reset calibration). This commit adds the control > of RESCAL as well as the ability to start and stop its > operation for PCIe HW. > > Signed-off-by: Jim Quinlan > --- > drivers/pci/controller/pcie-brcmstb.c | 81 ++- > 1 file changed, 80 insertions(+), 1 deletion(-) > > diff --git a/drivers/pci/controller/pcie-brcmstb.c > b/drivers/pci/controller/pcie-brcmstb.c > index 2c470104ba38..0787e8f6f7e5 100644 > --- a/drivers/pci/controller/pcie-brcmstb.c > +++ b/drivers/pci/controller/pcie-brcmstb.c [...] > @@ -1100,6 +1164,21 @@ static int brcm_pcie_probe(struct platform_device > *pdev) > dev_err(>dev, "could not enable clock\n"); > return ret; > } > + pcie->rescal = devm_reset_control_get_shared(>dev, "rescal"); > + if (IS_ERR(pcie->rescal)) { > + if (PTR_ERR(pcie->rescal) == -EPROBE_DEFER) > + return -EPROBE_DEFER; > + pcie->rescal = NULL; This is effectively an optional reset control, so it is better to use: ↵ pcie->rescal = devm_reset_control_get_optional_shared(>dev, "rescal");↵ if (IS_ERR(pcie->rescal)) return PTR_ERR(pcie->rescal); > + } else { > + ret = reset_control_deassert(pcie->rescal); > + if (ret) > + dev_err(>dev, "failed to deassert 'rescal'\n"); > + } reset_control_* can handle rstc == NULL parameters for optional reset controls, so this can be done unconditionally: ret = reset_control_deassert(pcie->rescal);↵ if (ret)↵ dev_err(>dev, "failed to deassert 'rescal'\n");↵ Is rescal handled by the reset-brcmstb-rescal driver? Since that only implements the .reset op, I would expect reset_control_reset() here. Otherwise this looks like it'd be missing a reset_control_assert in remove. regards Philipp